Virtualization - Dec 2017 - [RFC PATCH] virtio_net: Extend virtio to use VF datapath when available

On 12/19/2017 7:47 AM, Michael S. Tsirkin wrote:
> I'll need to look at this more, in particular the feature
> bit is missing here. For now one question:
>
> On Mon, Dec 18, 2017 at 04:40:36PM -0800, Sridhar Samudrala wrote:
>> @@ -56,6 +58,8 @@ module_param(napi_tx, bool, 0644);
>>    */
>>   DECLARE_EWMA(pkt_len, 0, 64)
>>   
>> +#define VF_TAKEOVER_INT	(HZ / 10)
>> +
>>   #define VIRTNET_DRIVER_VERSION "1.0.0"
>>   
>>   static const unsigned long guest_offloads[] = {
> Why is this delay necessary? And why by 100ms?
This is based on netvsc implementation and here is the commit that
added this delay.? Not sure if this needs to be 100ms.

commit 6123c66854c174e4982f98195100c1d990f9e5e6
Author: stephen hemminger <stephen at networkplumber.org>
Date:?? Wed Aug 9 17:46:03 2017 -0700

 ??? netvsc: delay setup of VF device

 ??? When VF device is discovered, delay bring it automatically up in
 ??? order to allow userspace to some simple changes (like renaming).

On Tue, 19 Dec 2017 09:41:39 -0800
"Samudrala, Sridhar" <sridhar.samudrala at intel.com> wrote:
> On 12/19/2017 7:47 AM, Michael S. Tsirkin wrote:
> > I'll need to look at this more, in particular the feature
> > bit is missing here. For now one question:
> >
> > On Mon, Dec 18, 2017 at 04:40:36PM -0800, Sridhar Samudrala wrote:  
> >> @@ -56,6 +58,8 @@ module_param(napi_tx, bool, 0644);
> >>    */
> >>   DECLARE_EWMA(pkt_len, 0, 64)
> >>   
> >> +#define VF_TAKEOVER_INT	(HZ / 10)
> >> +
> >>   #define VIRTNET_DRIVER_VERSION "1.0.0"
> >>   
> >>   static const unsigned long guest_offloads[] = {  
> > Why is this delay necessary? And why by 100ms?  
> 
> This is based on netvsc implementation and here is the commit that
> added this delay.? Not sure if this needs to be 100ms.
> 
> commit 6123c66854c174e4982f98195100c1d990f9e5e6
> Author: stephen hemminger <stephen at networkplumber.org>
> Date:?? Wed Aug 9 17:46:03 2017 -0700
> 
>  ??? netvsc: delay setup of VF device
> 
>  ??? When VF device is discovered, delay bring it automatically up in
>  ??? order to allow userspace to some simple changes (like renaming).
> 
> 
> 
could be 10ms, just enough to let udev do its renaming

On Tue, Dec 19, 2017 at 09:55:48AM -0800, Stephen Hemminger
wrote:
> On Tue, 19 Dec 2017 09:41:39 -0800
> "Samudrala, Sridhar" <sridhar.samudrala at intel.com>
wrote:
> 
> > On 12/19/2017 7:47 AM, Michael S. Tsirkin wrote:
> > > I'll need to look at this more, in particular the feature
> > > bit is missing here. For now one question:
> > >
> > > On Mon, Dec 18, 2017 at 04:40:36PM -0800, Sridhar Samudrala
wrote:
> > >> @@ -56,6 +58,8 @@ module_param(napi_tx, bool, 0644);
> > >>    */
> > >>   DECLARE_EWMA(pkt_len, 0, 64)
> > >>   
> > >> +#define VF_TAKEOVER_INT	(HZ / 10)
> > >> +
> > >>   #define VIRTNET_DRIVER_VERSION "1.0.0"
> > >>   
> > >>   static const unsigned long guest_offloads[] = {  
> > > Why is this delay necessary? And why by 100ms?  
> > 
> > This is based on netvsc implementation and here is the commit that
> > added this delay.? Not sure if this needs to be 100ms.
> > 
> > commit 6123c66854c174e4982f98195100c1d990f9e5e6
> > Author: stephen hemminger <stephen at networkplumber.org>
> > Date:?? Wed Aug 9 17:46:03 2017 -0700
> > 
> >  ??? netvsc: delay setup of VF device
> > 
> >  ??? When VF device is discovered, delay bring it automatically up in
> >  ??? order to allow userspace to some simple changes (like renaming).
> > 
> > 
> > 
> 
> could be 10ms, just enough to let udev do its renaming
Isn't there a way not to depend on udev completing its thing within a given
timeframe?

From: "Samudrala, Sridhar" <sridhar.samudrala at intel.com>
Date: Tue, 19 Dec 2017 09:41:39 -0800
> This is based on netvsc implementation and here is the commit that
> added this delay.? Not sure if this needs to be 100ms.
> 
> commit 6123c66854c174e4982f98195100c1d990f9e5e6
> Author: stephen hemminger <stephen at networkplumber.org>
> Date:?? Wed Aug 9 17:46:03 2017 -0700
> 
> ??? netvsc: delay setup of VF device
> 
> ??? When VF device is discovered, delay bring it automatically up in
> ??? order to allow userspace to some simple changes (like renaming).
This is kind of bogus, I should have called this out when the patch
was posted.

Any delay is wrong, there needs to be tight synchronization if a
userspace operation must occur before proceeding.  If something
happens and userspace is delayed, this whole thing doesn't work.

From: Stephen Hemminger <stephen at networkplumber.org>
Date: Tue, 19 Dec 2017 09:55:48 -0800
> could be 10ms, just enough to let udev do its renaming
Please, move to some kind of notification or event based handling of
this problem.

No delay is safe, what if userspace gets swapped out or whatever
else might make userspace stall unexpectedly?

David Miller <davem at davemloft.net> writes:
> From: "Samudrala, Sridhar" <sridhar.samudrala at intel.com>
> Date: Tue, 19 Dec 2017 09:41:39 -0800
>
>> This is based on netvsc implementation and here is the commit that
>> added this delay.? Not sure if this needs to be 100ms.
>> 
>> commit 6123c66854c174e4982f98195100c1d990f9e5e6
>> Author: stephen hemminger <stephen at networkplumber.org>
>> Date:?? Wed Aug 9 17:46:03 2017 -0700
>> 
>> ??? netvsc: delay setup of VF device
>> 
>> ??? When VF device is discovered, delay bring it automatically up in
>> ??? order to allow userspace to some simple changes (like renaming).
>
> This is kind of bogus, I should have called this out when the patch
> was posted.
>
> Any delay is wrong, there needs to be tight synchronization if a
> userspace operation must occur before proceeding.  If something
> happens and userspace is delayed, this whole thing doesn't work.
Hyper-V's netvsc does exactly the same and when this was first discussed
I suggested to allow renaming of IFF_UP interfaces:
https://patchwork.kernel.org/patch/9890299/

but as far as I understand it was decided that it's too risky and not
worth it. Maybe we need to reconsider this, at least for slave
interfaces (as scripts are not supposed to operate on them).

-- 
  Vitaly