Re: [Xen-devel] [PATCH V2 2/2] xenbus: defer xenbus frontend resume if xenstored is not running

[Aurelien Chartier <aurelien.chartier@xxxxxxxxxx>]

On 08/05/13 15:50, Jan Beulich wrote:
>>>> On 08.05.13 at 16:32, Aurelien Chartier <aurelien.chartier@xxxxxxxxxx> 
>>>> wrote:
>> --- a/drivers/xen/xenbus/xenbus_probe.h
>> +++ b/drivers/xen/xenbus/xenbus_probe.h
>> @@ -47,6 +47,11 @@ struct xen_bus_type {
>>      struct bus_type bus;
>>  };
>>  
>> +struct xenbus_resume_work {
>> +    struct work_struct w;
>> +    struct device *dev;
>> +};
>> +
> I don't think this structure needs to be in a header - it's being used
> in a single source file only.
>
>> --- a/drivers/xen/xenbus/xenbus_probe_frontend.c
>> +++ b/drivers/xen/xenbus/xenbus_probe_frontend.c
>> @@ -28,6 +28,7 @@
>>  #include "xenbus_comms.h"
>>  #include "xenbus_probe.h"
>>  
>> +static struct workqueue_struct *xenbus_frontend_resume_wq;
>>  
>>  /* device/<type>/<id> => <type>-<id> */
>>  static int frontend_bus_id(char bus_id[XEN_BUS_ID_SIZE], const char 
>> *nodename)
>> @@ -89,9 +90,38 @@ static void backend_changed(struct xenbus_watch *watch,
>>      xenbus_otherend_changed(watch, vec, len, 1);
>>  }
>>  
>> +static void xenbus_frontend_delayed_resume(struct work_struct *w)
>> +{
>> +    struct xenbus_resume_work *resume_work = (struct xenbus_resume_work *) 
>> w;
> container_of()
>
>> +
>> +    xenbus_dev_resume(dev);
> Does this build at all? I don't see where "dev" is being declared/
> initialized?
I messed up my config file and ended up with CONFIG_XEN_XENBUS_FRONTEND
wiped out, so I did not catch this one. Sorry for that, it was meant to
be resume_work->dev.

>
>> +
>> +    kfree(w);
> kfree(resume_work) - otherwise you have a hidden dependency
> on "w" being the first member of struct xenbus_resume_work.
>
>> +}
>> +
>> +static int xenbus_frontend_dev_resume(struct device *dev)
>> +{
>> +    /* 
>> +     * If xenstored is running in that domain, we cannot access the backend
>> +     * state at the moment, so we need to defer xenbus_dev_resume
>> +     */
>> +    if (xen_store_domain == XS_LOCAL) {
>> +            struct xenbus_resume_work *work =
>> +                    kmalloc(sizeof(struct xenbus_resume), GFP_KERNEL);
> Missing NULL return check (don't know what you should do in that
> case). Perhaps dynamic allocation is the wrong approach here, and
> you want to rather add a new field to struct xenbus_device (which
> gets populated only for frontend devices, and - if possible - only
> when xen_store_domain == XS_LOCAL.
>
> Also - GFP_KERNEL really suitable here?
Adding a new field for xenbus_device for that purpose seemed too much
for me, but it is probably a cleaner solution in the end.

I don't really see a way to avoid having that field for backend devices.
We can have a #ifdef CONFIG_XEN_XENBUS_FRONTEND, but that won't prevent
domains with both frontend and backend to have that field set in all
xenbus devices.

>
>> +
>> +            INIT_WORK((struct work_struct *) work, 
>> xenbus_frontend_delayed_resume);
> &work->w (without any cast).
>
>> +            resume_work->dev = dev;
>> +            queue_work(xenbus_frontend_resume_wq, (struct work_struct *) 
>> work)
> Again.

Thanks for the review. I will fix the errors in my next patch series.

Aurelien.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel