Re: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and Rescind offer

[Jason Wang <jasowang@redhat.com>]

On Wed, Jan 28, 2015 at 9:09 PM, Vitaly Kuznetsov <vkuznets@redhat.com> 
wrote:
> Dexuan Cui <decui@microsoft.com> writes:
> 
>>>  -----Original Message-----
>>>  From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
>>>  Sent: Wednesday, January 28, 2015 20:09 PM
>>>  To: Dexuan Cui
>>>  Cc: KY Srinivasan; devel@linuxdriverproject.org; Haiyang Zhang; 
>>> linux-
>>>  kernel@vger.kernel.org; Jason Wang; Radim Krčmář; Dan Carpenter
>>>  Subject: Re: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer 
>>> and Rescind
>>>  offer
>>>  
>>>  Dexuan Cui <decui@microsoft.com> writes:
>>>  
>>>  >> -----Original Message-----
>>>  >> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
>>>  >> Sent: Tuesday, January 20, 2015 23:45 PM
>>>  >> To: KY Srinivasan; devel@linuxdriverproject.org
>>>  >> Cc: Haiyang Zhang; linux-kernel@vger.kernel.org; Dexuan Cui; 
>>> Jason Wang;
>>>  >> Radim Krčmář; Dan Carpenter
>>>  >> Subject: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and 
>>> Rescind
>>>  offer
>>>  ...
>>>  >
>>>  > Hi Vitaly and all,
>>>  > I have 2 questions:
>>>  > In vmbus_process_offer(),  in the cases of "goto err_free_chan",
>>>  > should we consider the possibility a rescind message could be 
>>> pending for
>>>  > the new channel?
>>>  > In the cases,  because we don't run
>>>  > "INIT_WORK(&newchannel->work, vmbus_process_rescind_offer); ",
>>>  > vmbus_onoffer_rescind() will do nothing and as a result,
>>>  > vmbus_process_rescind_offer() won't be invoked.
>>>  
>>>  Yes, but processing the rescind offer results in freeing the 
>>> channel
>>>  (and this processing supposes the channel wasn't freed before) so
>>>  there is no difference... or is it?
>>>  
>>>  >
>>>  > Question 2: in vmbus_process_offer(), in the case
>>>  > vmbus_device_register() fails, we'll run
>>>  > "list_del(&newchannel->listentry);" -- just after this line,
>>>  >  what will happen at this time if relid2channel() returns NULL
>>>  > in vmbus_onoffer_rescind()?
>>>  >
>>>  > I think we'll lose the rescind message.
>>>  >
>>>  
>>>  Yes, but same logic applies - we already freed the channes so no 
>>> rescind
>>>  proccessing required.
>>  free_channel() and vmbus_process_rescind_offer() are different, 
>> because
>>   the latter does more work, e.g., sending the host a message 
>>  CHANNELMSG_RELID_RELEASED.
>> 
>>  In the cases of "goto err_free_chan" + "a pending rescind message",
>>  the host may expect the message CHANNELMSG_RELID_RELEASED and
>>  could reoffer the channel once the message is received.
>> 
>>  It would be better if the VM doesn't lose the rescind message
>>  here. :-)
> 
> Ah, I see, CHANNELMSG_RELID_RELEASED is expected from us in any
> case. I'll doing that in a separate patch is noone objects.

All the evil come from the un-serialized processing of message. I 
wonder whether we can do all the processing in workqueue and then those 
were automatically serialized.

> 
> Thanks for the review,
> 
>> 
>>>   If we still need to do something we need to
>>>  add support for already freed channel to the rescind offer 
>>> processing path.
>>>  
>> 
>>  Thanks,
>>  -- Dexuan
> 
> -- 
>   Vitaly
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/