Re: [Xen-devel] [PATCH] xen-netfront: wait xenbus state change when load module manually

[Jiri Slaby]

On 08/24/2018, 04:26 PM, Boris Ostrovsky wrote:
> On 08/24/2018 07:26 AM, Juergen Gross wrote:
>> On 24/08/18 13:12, Jiri Slaby wrote:
>>> On 07/30/2018, 10:18 AM, Xiao Liang wrote:
>>>> On 07/29/2018 11:30 PM, David Miller wrote:
>>>>> From: Xiao Liang <xiliang@xxxxxxxxxx>
>>>>> Date: Fri, 27 Jul 2018 17:56:08 +0800
>>>>>
>>>>>> @@ -1330,6 +1331,11 @@ static struct net_device
>>>>>> *xennet_create_dev(struct xenbus_device *dev)
>>>>>> ÂÂÂÂÂ netif_carrier_off(netdev);
>>>>>> Â ÂÂÂÂÂ xenbus_switch_state(dev, XenbusStateInitialising);
>>>>>> +ÂÂÂ wait_event(module_load_q,
>>>>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ xenbus_read_driver_state(dev->otherend) !=
>>>>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ XenbusStateClosed &&
>>>>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ xenbus_read_driver_state(dev->otherend) !=
>>>>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ XenbusStateUnknown);
>>>>>> ÂÂÂÂÂ return netdev;
>>>>>> Â ÂÂ exit:
>>>>> What performs the wakeups that will trigger for this sleep site?
>>>> In my understanding, backend leaving closed/unknow state can trigger the
>>>> wakeups. I mean to make sure both sides are ready for creating connection.
>>> While backporting this to 4.12, I was surprised by the commit the same
>>> as Boris and David.
>>>
>>> So I assume the explanation is that wake_up_all of module_unload_q in
>>> netback_changed wakes also all the processes waiting on module_load_q?
>>> If so, what makes sure that module_unload_q is queued and the process is
>>> the same as for module_load_q?
>> How could it? Either the thread is waiting on module_unload_q _or_ on
>> module_load_q. It can't wait on two queues at the same time.
>>
>>> To me, it looks rather error-prone. Unless it is erroneous now, at least
>>> for future changes. Wouldn't it make sense to wake up module_load_q
>>> along with module_unload_q in netback_changed? Or drop module_load_q
>>> completely and use only module_unload_q (i.e. in xennet_create_dev too)?
>> To me this looks just wrong. A thread waiting on module_load_q won't be
>> woken up again.
>>
>> I'd drop module_load_q in favor of module_unload_q.
> 
> 
> Yes, use single queue, but rename it to something more neutral. module_wq?

Can somebody who is actually using the module fix this, please?

I could fix it, but untested changes are "a bit" worse than tested changes.

thanks,
-- 
js
suse labs