qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 1/4] Introduce yank feature


From: Stefan Hajnoczi
Subject: Re: [PATCH v2 1/4] Introduce yank feature
Date: Wed, 17 Jun 2020 15:39:36 +0100

On Thu, May 21, 2020 at 04:48:06PM +0100, Daniel P. Berrangé wrote:
> On Thu, May 21, 2020 at 05:42:41PM +0200, Lukas Straub wrote:
> > On Thu, 21 May 2020 16:03:35 +0100
> > Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > 
> > > On Wed, May 20, 2020 at 11:05:39PM +0200, Lukas Straub wrote:
> > > > +void yank_generic_iochannel(void *opaque)
> > > > +{
> > > > +    QIOChannel *ioc = QIO_CHANNEL(opaque);
> > > > +
> > > > +    qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
> > > > +}
> > > > +
> > > > +void qmp_yank(strList *instances, Error **errp)
> > > > +{
> > > > +    strList *tmp;
> > > > +    struct YankInstance *instance;
> > > > +    struct YankFuncAndParam *entry;
> > > > +
> > > > +    qemu_mutex_lock(&lock);
> > > > +    tmp = instances;
> > > > +    for (; tmp; tmp = tmp->next) {
> > > > +        instance = yank_find_instance(tmp->value);
> > > > +        if (!instance) {
> > > > +            error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> > > > +                      "Instance '%s' not found", tmp->value);
> > > > +            qemu_mutex_unlock(&lock);
> > > > +            return;
> > > > +        }
> > > > +    }
> > > > +    tmp = instances;
> > > > +    for (; tmp; tmp = tmp->next) {
> > > > +        instance = yank_find_instance(tmp->value);
> > > > +        assert(instance);
> > > > +        QLIST_FOREACH(entry, &instance->yankfns, next) {
> > > > +            entry->func(entry->opaque);
> > > > +        }
> > > > +    }
> > > > +    qemu_mutex_unlock(&lock);
> > > > +}  
> > > 
> > > From docs/devel/qapi-code-gen.txt:
> > > 
> > >   An OOB-capable command handler must satisfy the following conditions:
> > > 
> > >   - It terminates quickly.
> > Check.
> > 
> > >   - It does not invoke system calls that may block.
> > brk/sbrk (malloc and friends):
> > The manpage doesn't say anything about blocking, but malloc is already used 
> > while handling the qmp command.
> > 
> > shutdown():
> > The manpage doesn't say anything about blocking, but this is already used 
> > in migration oob qmp commands.
> 
> It just marks the socket state in local kernel side. It doesn't involve
> any blocking roundtrips over the wire, so this is fine.
> 
> > 
> > There are no other syscalls involved to my knowledge.
> > 
> > >   - It does not access guest RAM that may block when userfaultfd is
> > >     enabled for postcopy live migration.
> > Check.
> > 
> > >   - It takes only "fast" locks, i.e. all critical sections protected by
> > >     any lock it takes also satisfy the conditions for OOB command
> > >     handler code.
> > 
> > The lock in yank.c satisfies this requirement.
> > 
> > qio_channel_shutdown doesn't take any locks.
> 
> Agreed, I think the yank code is compliant with all the points
> listed above.

The code does not document this in any way. In fact, it's an interface
for registering arbitrary callback functions which execute in this
special environment.

If you don't document this explicitly it will break when someone changes
the code, even if it works right now.

Please document this in the yank APIs and also in the implementation.

For example, QemuMutex yank has the priority inversion problem: no other
function may violate the oob rules while holding the mutex, otherwise
the oob function will hang while waiting for the lock when the other
function is blocked.

Stefan

Attachment: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]