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

[Lukas Straub]

On Wed, 17 Jun 2020 15:39:36 +0100
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> 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.

Hi,
It is documented in yank.h:

/**
 * yank_register_function: Register a yank function
 *
 * This registers a yank function. All limitations of qmp oob commands apply
 * to the yank function as well.
 *
 * This function is thread-safe.
 *
 * @instance_name: The name of the instance
 * @func: The yank function
 * @opaque: Will be passed to the yank function
 */

Thanks,
Lukas Straub

> 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

pgpsQ_BH1HtWT.pgp
Description: OpenPGP digital signature