Re: [PATCH net-next v11 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Samudrala, Sridhar]

On 5/22/2018 11:27 PM, Jiri Pirko wrote:

Tue, May 22, 2018 at 10:54:29PM CEST, sridhar.samudr...@intel.com wrote:


On 5/22/2018 9:12 AM, Jiri Pirko wrote:

Fixing the subj, sorry about that.

Tue, May 22, 2018 at 05:46:21PM CEST, m...@redhat.com wrote:

On Tue, May 22, 2018 at 05:36:14PM +0200, Jiri Pirko wrote:

Tue, May 22, 2018 at 05:28:42PM CEST, sridhar.samudr...@intel.com wrote:

On 5/22/2018 2:08 AM, Jiri Pirko wrote:

Tue, May 22, 2018 at 11:06:37AM CEST, j...@resnulli.us wrote:

Tue, May 22, 2018 at 04:06:18AM CEST, sridhar.samudr...@intel.com wrote:

Use the registration/notification framework supported by the generic
failover infrastructure.

Signed-off-by: Sridhar Samudrala 

In previous patchset versions, the common code did
netdev_rx_handler_register() and netdev_upper_dev_link() etc
(netvsc_vf_join()). Now, this is still done in netvsc. Why?

This should be part of the common "failover" code.

Based on Stephen's feedback on earlier patches, i tried to minimize the changes 
to
netvsc and only commonize the notifier and the main event handler routine.
Another complication is that netvsc does part of registration in a delayed 
workqueue.

:( This kind of degrades the whole efford of having single solution
in "failover" module. I think that common parts, as
netdev_rx_handler_register() and others certainly is should be inside
the common module. This is not a good time to minimize changes. Let's do
the thing properly and fix the netvsc mess now.



It should be possible to move some of the code from net_failover.c to generic
failover.c in future if Stephen is ok with it.



Also note that in the current patchset you use IFF_FAILOVER flag for
master, yet for the slave you use IFF_SLAVE. That is wrong.
IFF_FAILOVER_SLAVE should be used.

Not sure which code you are referring to.  I only set IFF_FAILOVER_SLAVE
in patch 3.

The existing netvsc driver.

We really can't change netvsc's flags now, even if it's interface is
messy, it's being used in the field. We can add a flag that makes netvsc
behave differently, and if this flag also allows enhanced functionality
userspace will gradually switch.

Okay, although in this case, it really does not make much sense, so be
it. Leave the netvsc set the ->priv flag to IFF_SLAVE as it is doing
now. (This once-wrong-forever-wrong policy is flustrating me).

But since this patchset introduces private flag IFF_FAILOVER and
IFF_FAILOVER_SLAVE, and we set IFF_FAILOVER to the netvsc netdev
instance, we should also set IFF_FAILOVER_SLAVE to the enslaved VF
netdevice to get at least some consistency between virtio_net and
netvsc.

OK. I can make this change to set/unset IFF_FAILOVER_SLAVE in the netvsc
register/unregister routines so that it is consistent with virtio_net.

Based on your discussion with mst, i think we can even remove IFF_SLAVE
setting on netvsc as it should not impact userspace.  If Stephen is OK
we can make this change too.

Do you see any other items that need to be resolved for this series to go in
this merge window?

As I wrote previously, the common code including rx_handler registration
and setting of flags and master link should be done in a common code,
moved away from netvsc code.


This requires re-introducing the 2 additional ops pre_register and 
pre_unregister
that i removed in the last couple of revisions to minimize netvsc changes and 
the
indirect calls that Stephen expressed some concern.

But, as these calls don't happen in hot path, i guess it should not be a big
issue and the right way to go.
Will submit a v12 with these updates.

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v11 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Jiri Pirko]

Tue, May 22, 2018 at 10:54:29PM CEST, sridhar.samudr...@intel.com wrote:
>
>
>On 5/22/2018 9:12 AM, Jiri Pirko wrote:
>> Fixing the subj, sorry about that.
>> 
>> Tue, May 22, 2018 at 05:46:21PM CEST, m...@redhat.com wrote:
>> > On Tue, May 22, 2018 at 05:36:14PM +0200, Jiri Pirko wrote:
>> > > Tue, May 22, 2018 at 05:28:42PM CEST, sridhar.samudr...@intel.com wrote:
>> > > > On 5/22/2018 2:08 AM, Jiri Pirko wrote:
>> > > > > Tue, May 22, 2018 at 11:06:37AM CEST, j...@resnulli.us wrote:
>> > > > > > Tue, May 22, 2018 at 04:06:18AM CEST, sridhar.samudr...@intel.com 
>> > > > > > wrote:
>> > > > > > > Use the registration/notification framework supported by the 
>> > > > > > > generic
>> > > > > > > failover infrastructure.
>> > > > > > > 
>> > > > > > > Signed-off-by: Sridhar Samudrala 
>> > > > > > In previous patchset versions, the common code did
>> > > > > > netdev_rx_handler_register() and netdev_upper_dev_link() etc
>> > > > > > (netvsc_vf_join()). Now, this is still done in netvsc. Why?
>> > > > > > 
>> > > > > > This should be part of the common "failover" code.
>> > > > Based on Stephen's feedback on earlier patches, i tried to minimize 
>> > > > the changes to
>> > > > netvsc and only commonize the notifier and the main event handler 
>> > > > routine.
>> > > > Another complication is that netvsc does part of registration in a 
>> > > > delayed workqueue.
>> > > :( This kind of degrades the whole efford of having single solution
>> > > in "failover" module. I think that common parts, as
>> > > netdev_rx_handler_register() and others certainly is should be inside
>> > > the common module. This is not a good time to minimize changes. Let's do
>> > > the thing properly and fix the netvsc mess now.
>> > > 
>> > > 
>> > > > It should be possible to move some of the code from net_failover.c to 
>> > > > generic
>> > > > failover.c in future if Stephen is ok with it.
>> > > > 
>> > > > 
>> > > > > Also note that in the current patchset you use IFF_FAILOVER flag for
>> > > > > master, yet for the slave you use IFF_SLAVE. That is wrong.
>> > > > > IFF_FAILOVER_SLAVE should be used.
>> > > > Not sure which code you are referring to.  I only set 
>> > > > IFF_FAILOVER_SLAVE
>> > > > in patch 3.
>> > > The existing netvsc driver.
>> > We really can't change netvsc's flags now, even if it's interface is
>> > messy, it's being used in the field. We can add a flag that makes netvsc
>> > behave differently, and if this flag also allows enhanced functionality
>> > userspace will gradually switch.
>> Okay, although in this case, it really does not make much sense, so be
>> it. Leave the netvsc set the ->priv flag to IFF_SLAVE as it is doing
>> now. (This once-wrong-forever-wrong policy is flustrating me).
>> 
>> But since this patchset introduces private flag IFF_FAILOVER and
>> IFF_FAILOVER_SLAVE, and we set IFF_FAILOVER to the netvsc netdev
>> instance, we should also set IFF_FAILOVER_SLAVE to the enslaved VF
>> netdevice to get at least some consistency between virtio_net and
>> netvsc.
>
>OK. I can make this change to set/unset IFF_FAILOVER_SLAVE in the netvsc
>register/unregister routines so that it is consistent with virtio_net.
>
>Based on your discussion with mst, i think we can even remove IFF_SLAVE
>setting on netvsc as it should not impact userspace.  If Stephen is OK
>we can make this change too.
>
>Do you see any other items that need to be resolved for this series to go in
>this merge window?

As I wrote previously, the common code including rx_handler registration
and setting of flags and master link should be done in a common code,
moved away from netvsc code.

Thanks.


>
>
>
>> 
>> > Anything breaking userspace I fully expect Stephen to nack and
>> > IMO with good reason.
>> > 
>> > -- 
>> > MST
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v11 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Samudrala, Sridhar]

On 5/22/2018 9:12 AM, Jiri Pirko wrote:

Fixing the subj, sorry about that.

Tue, May 22, 2018 at 05:46:21PM CEST, m...@redhat.com wrote:

On Tue, May 22, 2018 at 05:36:14PM +0200, Jiri Pirko wrote:

Tue, May 22, 2018 at 05:28:42PM CEST, sridhar.samudr...@intel.com wrote:

On 5/22/2018 2:08 AM, Jiri Pirko wrote:

Tue, May 22, 2018 at 11:06:37AM CEST, j...@resnulli.us wrote:

Tue, May 22, 2018 at 04:06:18AM CEST, sridhar.samudr...@intel.com wrote:

Use the registration/notification framework supported by the generic
failover infrastructure.

Signed-off-by: Sridhar Samudrala 

In previous patchset versions, the common code did
netdev_rx_handler_register() and netdev_upper_dev_link() etc
(netvsc_vf_join()). Now, this is still done in netvsc. Why?

This should be part of the common "failover" code.

Based on Stephen's feedback on earlier patches, i tried to minimize the changes 
to
netvsc and only commonize the notifier and the main event handler routine.
Another complication is that netvsc does part of registration in a delayed 
workqueue.

:( This kind of degrades the whole efford of having single solution
in "failover" module. I think that common parts, as
netdev_rx_handler_register() and others certainly is should be inside
the common module. This is not a good time to minimize changes. Let's do
the thing properly and fix the netvsc mess now.



It should be possible to move some of the code from net_failover.c to generic
failover.c in future if Stephen is ok with it.



Also note that in the current patchset you use IFF_FAILOVER flag for
master, yet for the slave you use IFF_SLAVE. That is wrong.
IFF_FAILOVER_SLAVE should be used.

Not sure which code you are referring to.  I only set IFF_FAILOVER_SLAVE
in patch 3.

The existing netvsc driver.

We really can't change netvsc's flags now, even if it's interface is
messy, it's being used in the field. We can add a flag that makes netvsc
behave differently, and if this flag also allows enhanced functionality
userspace will gradually switch.

Okay, although in this case, it really does not make much sense, so be
it. Leave the netvsc set the ->priv flag to IFF_SLAVE as it is doing
now. (This once-wrong-forever-wrong policy is flustrating me).

But since this patchset introduces private flag IFF_FAILOVER and
IFF_FAILOVER_SLAVE, and we set IFF_FAILOVER to the netvsc netdev
instance, we should also set IFF_FAILOVER_SLAVE to the enslaved VF
netdevice to get at least some consistency between virtio_net and
netvsc.


OK. I can make this change to set/unset IFF_FAILOVER_SLAVE in the netvsc
register/unregister routines so that it is consistent with virtio_net.

Based on your discussion with mst, i think we can even remove IFF_SLAVE
setting on netvsc as it should not impact userspace.  If Stephen is OK
we can make this change too.

Do you see any other items that need to be resolved for this series to go in
this merge window?






Anything breaking userspace I fully expect Stephen to nack and
IMO with good reason.

--
MST


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v11 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Michael S. Tsirkin]

On Tue, May 22, 2018 at 07:38:44PM +0200, Jiri Pirko wrote:
> >> >> In private
> >> >> flag. I don't see no reason to break this pattern here.
> >> >
> >> >Other masters are setup from userspace, this one is set up automatically
> >> >by kernel. So the bar is higher, we need an interface that existing
> >> >userspace knows about.  We can't just say "oh if userspace set this up
> >> >it should know to skip lowerdevs".
> >> >
> >> >Otherwise multiple interfaces with same mac tend to confuse userspace.
> >> 
> >> No difference, really.
> >> Regardless who does the setup, and independent userspace deamon should
> >> react accordingly.
> >
> >If the deamon does the setup itself, it's reasonable to require that it
> >learns about new flags each time we add a new driver.  If it doesn't,
> >then I think it's less reasonable.
> 
> No need. The "IFLA_MASTER" attr is always there to be looked at. That is
> enough.

Oh so if it has an master, skip it? Sorry, I misunderstood what you were
saying earlier.

Thanks, this makes sense to me.

-- 
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v11 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Jiri Pirko]

Tue, May 22, 2018 at 06:52:21PM CEST, m...@redhat.com wrote:
>On Tue, May 22, 2018 at 05:45:01PM +0200, Jiri Pirko wrote:
>> Tue, May 22, 2018 at 05:32:30PM CEST, m...@redhat.com wrote:
>> >On Tue, May 22, 2018 at 05:13:43PM +0200, Jiri Pirko wrote:
>> >> Tue, May 22, 2018 at 03:39:33PM CEST, m...@redhat.com wrote:
>> >> >On Tue, May 22, 2018 at 03:26:26PM +0200, Jiri Pirko wrote:
>> >> >> Tue, May 22, 2018 at 03:17:37PM CEST, m...@redhat.com wrote:
>> >> >> >On Tue, May 22, 2018 at 03:14:22PM +0200, Jiri Pirko wrote:
>> >> >> >> Tue, May 22, 2018 at 03:12:40PM CEST, m...@redhat.com wrote:
>> >> >> >> >On Tue, May 22, 2018 at 11:08:53AM +0200, Jiri Pirko wrote:
>> >> >> >> >> Tue, May 22, 2018 at 11:06:37AM CEST, j...@resnulli.us wrote:
>> >> >> >> >> >Tue, May 22, 2018 at 04:06:18AM CEST, 
>> >> >> >> >> >sridhar.samudr...@intel.com wrote:
>> >> >> >> >> >>Use the registration/notification framework supported by the 
>> >> >> >> >> >>generic
>> >> >> >> >> >>failover infrastructure.
>> >> >> >> >> >>
>> >> >> >> >> >>Signed-off-by: Sridhar Samudrala 
>> >> >> >> >> >
>> >> >> >> >> >In previous patchset versions, the common code did
>> >> >> >> >> >netdev_rx_handler_register() and netdev_upper_dev_link() etc
>> >> >> >> >> >(netvsc_vf_join()). Now, this is still done in netvsc. Why?
>> >> >> >> >> >
>> >> >> >> >> >This should be part of the common "failover" code.
>> >> >> >> >> >
>> >> >> >> >> 
>> >> >> >> >> Also note that in the current patchset you use IFF_FAILOVER flag 
>> >> >> >> >> for
>> >> >> >> >> master, yet for the slave you use IFF_SLAVE. That is wrong.
>> >> >> >> >> IFF_FAILOVER_SLAVE should be used.
>> >> >> >> >
>> >> >> >> >Or drop IFF_FAILOVER_SLAVE and set both IFF_FAILOVER and IFF_SLAVE?
>> >> >> >> 
>> >> >> >> No. IFF_SLAVE is for bonding.
>> >> >> >
>> >> >> >What breaks if we reuse it for failover?
>> >> >> 
>> >> >> This is exposed to userspace. IFF_SLAVE is expected for bonding slaves.
>> >> >> And failover slave is not a bonding slave.
>> >> >
>> >> >That does not really answer the question.  I'd claim it's sufficiently
>> >> >like a bond slave for IFF_SLAVE to make sense.
>> >> >
>> >> >In fact you will find that netvsc already sets IFF_SLAVE, and so
>> >> 
>> >> netvsc does the whole failover thing in a wrong way. This patchset is
>> >> trying to fix it.
>> >
>> >Maybe, but we don't need gratuitous changes either, especially if they
>> >break userspace.
>> 
>> What do you mean by the "break"? It was a mistake to reuse IFF_SLAVE at
>> the first place, lets fix it. If some userspace depends on that flag, it
>> is broken anyway.
>> 
>> 
>> >
>> >> >does e.g. the eql driver.
>> >> >
>> >> >The advantage of using IFF_SLAVE is that userspace knows to skip it.  If
>> >> 
>> >> The userspace should know how to skip other types of slaves - team,
>> >> bridge, ovs, etc.
>> >> The "master link" should be the one to look at.
>> >> 
>> >
>> >How should existing userspace know which ones to skip and which one is
>> >the master?  Right now userspace seems to assume whatever does not have
>> >IFF_SLAVE should be looked at. Are you saying that's not the right thing
>> 
>> Why do you say so? What do you mean by "looked at"? Certainly not.
>> IFLA_MASTER is the attribute that should be looked at, nothing else.
>> 
>> 
>> >to do and userspace should be fixed? What should userspace do in
>> >your opinion that will be forward compatible with future kernels?
>> >
>> >> 
>> >> >we don't set IFF_SLAVE existing userspace tries to use the lowerdev.
>> >> 
>> >> Each master type has a IFF_ master flag and IFF_ slave flag.
>> >
>> >Could you give some examples please?
>> 
>> enum netdev_priv_flags {
>> IFF_EBRIDGE = 1<<1,
>> IFF_BRIDGE_PORT = 1<<9,
>> IFF_OPENVSWITCH = 1<<20,
>> IFF_OVS_DATAPATH= 1<<10,
>>  IFF_L3MDEV_MASTER   = 1<<18,
>> IFF_L3MDEV_SLAVE= 1<<21,
>> IFF_TEAM= 1<<22,
>> IFF_TEAM_PORT   = 1<<13,
>> };
>
>That's not in uapi, is it?  the comment above that says:

Correct.


>
>These flags are invisible to userspace
>
>
>
>> 
>> >
>> >> In private
>> >> flag. I don't see no reason to break this pattern here.
>> >
>> >Other masters are setup from userspace, this one is set up automatically
>> >by kernel. So the bar is higher, we need an interface that existing
>> >userspace knows about.  We can't just say "oh if userspace set this up
>> >it should know to skip lowerdevs".
>> >
>> >Otherwise multiple interfaces with same mac tend to confuse userspace.
>> 
>> No difference, really.
>> Regardless who does the setup, and independent userspace deamon should
>> react accordingly.
>
>If the deamon does the setup itself, it's reasonable to require that it
>learns about new flags each time we add a new driver.  If it doesn't,
>then I think it's less reasonable.

No need. The 

Re: [PATCH net-next v11 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Michael S. Tsirkin]

On Tue, May 22, 2018 at 05:45:01PM +0200, Jiri Pirko wrote:
> Tue, May 22, 2018 at 05:32:30PM CEST, m...@redhat.com wrote:
> >On Tue, May 22, 2018 at 05:13:43PM +0200, Jiri Pirko wrote:
> >> Tue, May 22, 2018 at 03:39:33PM CEST, m...@redhat.com wrote:
> >> >On Tue, May 22, 2018 at 03:26:26PM +0200, Jiri Pirko wrote:
> >> >> Tue, May 22, 2018 at 03:17:37PM CEST, m...@redhat.com wrote:
> >> >> >On Tue, May 22, 2018 at 03:14:22PM +0200, Jiri Pirko wrote:
> >> >> >> Tue, May 22, 2018 at 03:12:40PM CEST, m...@redhat.com wrote:
> >> >> >> >On Tue, May 22, 2018 at 11:08:53AM +0200, Jiri Pirko wrote:
> >> >> >> >> Tue, May 22, 2018 at 11:06:37AM CEST, j...@resnulli.us wrote:
> >> >> >> >> >Tue, May 22, 2018 at 04:06:18AM CEST, 
> >> >> >> >> >sridhar.samudr...@intel.com wrote:
> >> >> >> >> >>Use the registration/notification framework supported by the 
> >> >> >> >> >>generic
> >> >> >> >> >>failover infrastructure.
> >> >> >> >> >>
> >> >> >> >> >>Signed-off-by: Sridhar Samudrala 
> >> >> >> >> >
> >> >> >> >> >In previous patchset versions, the common code did
> >> >> >> >> >netdev_rx_handler_register() and netdev_upper_dev_link() etc
> >> >> >> >> >(netvsc_vf_join()). Now, this is still done in netvsc. Why?
> >> >> >> >> >
> >> >> >> >> >This should be part of the common "failover" code.
> >> >> >> >> >
> >> >> >> >> 
> >> >> >> >> Also note that in the current patchset you use IFF_FAILOVER flag 
> >> >> >> >> for
> >> >> >> >> master, yet for the slave you use IFF_SLAVE. That is wrong.
> >> >> >> >> IFF_FAILOVER_SLAVE should be used.
> >> >> >> >
> >> >> >> >Or drop IFF_FAILOVER_SLAVE and set both IFF_FAILOVER and IFF_SLAVE?
> >> >> >> 
> >> >> >> No. IFF_SLAVE is for bonding.
> >> >> >
> >> >> >What breaks if we reuse it for failover?
> >> >> 
> >> >> This is exposed to userspace. IFF_SLAVE is expected for bonding slaves.
> >> >> And failover slave is not a bonding slave.
> >> >
> >> >That does not really answer the question.  I'd claim it's sufficiently
> >> >like a bond slave for IFF_SLAVE to make sense.
> >> >
> >> >In fact you will find that netvsc already sets IFF_SLAVE, and so
> >> 
> >> netvsc does the whole failover thing in a wrong way. This patchset is
> >> trying to fix it.
> >
> >Maybe, but we don't need gratuitous changes either, especially if they
> >break userspace.
> 
> What do you mean by the "break"? It was a mistake to reuse IFF_SLAVE at
> the first place, lets fix it. If some userspace depends on that flag, it
> is broken anyway.
> 
> 
> >
> >> >does e.g. the eql driver.
> >> >
> >> >The advantage of using IFF_SLAVE is that userspace knows to skip it.  If
> >> 
> >> The userspace should know how to skip other types of slaves - team,
> >> bridge, ovs, etc.
> >> The "master link" should be the one to look at.
> >> 
> >
> >How should existing userspace know which ones to skip and which one is
> >the master?  Right now userspace seems to assume whatever does not have
> >IFF_SLAVE should be looked at. Are you saying that's not the right thing
> 
> Why do you say so? What do you mean by "looked at"? Certainly not.
> IFLA_MASTER is the attribute that should be looked at, nothing else.
> 
> 
> >to do and userspace should be fixed? What should userspace do in
> >your opinion that will be forward compatible with future kernels?
> >
> >> 
> >> >we don't set IFF_SLAVE existing userspace tries to use the lowerdev.
> >> 
> >> Each master type has a IFF_ master flag and IFF_ slave flag.
> >
> >Could you give some examples please?
> 
> enum netdev_priv_flags {
> IFF_EBRIDGE = 1<<1,
> IFF_BRIDGE_PORT = 1<<9,
> IFF_OPENVSWITCH = 1<<20,
> IFF_OVS_DATAPATH= 1<<10,
>   IFF_L3MDEV_MASTER   = 1<<18,
> IFF_L3MDEV_SLAVE= 1<<21,
> IFF_TEAM= 1<<22,
> IFF_TEAM_PORT   = 1<<13,
> };

That's not in uapi, is it?  the comment above that says:

These flags are invisible to userspace



> 
> >
> >> In private
> >> flag. I don't see no reason to break this pattern here.
> >
> >Other masters are setup from userspace, this one is set up automatically
> >by kernel. So the bar is higher, we need an interface that existing
> >userspace knows about.  We can't just say "oh if userspace set this up
> >it should know to skip lowerdevs".
> >
> >Otherwise multiple interfaces with same mac tend to confuse userspace.
> 
> No difference, really.
> Regardless who does the setup, and independent userspace deamon should
> react accordingly.

If the deamon does the setup itself, it's reasonable to require that it
learns about new flags each time we add a new driver.  If it doesn't,
then I think it's less reasonable.

-- 
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v11 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Jiri Pirko]

Fixing the subj, sorry about that.

Tue, May 22, 2018 at 05:46:21PM CEST, m...@redhat.com wrote:
>On Tue, May 22, 2018 at 05:36:14PM +0200, Jiri Pirko wrote:
>> Tue, May 22, 2018 at 05:28:42PM CEST, sridhar.samudr...@intel.com wrote:
>> >
>> >On 5/22/2018 2:08 AM, Jiri Pirko wrote:
>> >> Tue, May 22, 2018 at 11:06:37AM CEST, j...@resnulli.us wrote:
>> >> > Tue, May 22, 2018 at 04:06:18AM CEST, sridhar.samudr...@intel.com wrote:
>> >> > > Use the registration/notification framework supported by the generic
>> >> > > failover infrastructure.
>> >> > > 
>> >> > > Signed-off-by: Sridhar Samudrala 
>> >> > In previous patchset versions, the common code did
>> >> > netdev_rx_handler_register() and netdev_upper_dev_link() etc
>> >> > (netvsc_vf_join()). Now, this is still done in netvsc. Why?
>> >> > 
>> >> > This should be part of the common "failover" code.
>> >
>> >Based on Stephen's feedback on earlier patches, i tried to minimize the 
>> >changes to
>> >netvsc and only commonize the notifier and the main event handler routine.
>> >Another complication is that netvsc does part of registration in a delayed 
>> >workqueue.
>> 
>> :( This kind of degrades the whole efford of having single solution
>> in "failover" module. I think that common parts, as
>> netdev_rx_handler_register() and others certainly is should be inside
>> the common module. This is not a good time to minimize changes. Let's do
>> the thing properly and fix the netvsc mess now.
>> 
>> 
>> >
>> >It should be possible to move some of the code from net_failover.c to 
>> >generic
>> >failover.c in future if Stephen is ok with it.
>> >
>> >
>> >> > 
>> >> Also note that in the current patchset you use IFF_FAILOVER flag for
>> >> master, yet for the slave you use IFF_SLAVE. That is wrong.
>> >> IFF_FAILOVER_SLAVE should be used.
>> >
>> >Not sure which code you are referring to.  I only set IFF_FAILOVER_SLAVE
>> >in patch 3.
>> 
>> The existing netvsc driver.
>
>We really can't change netvsc's flags now, even if it's interface is
>messy, it's being used in the field. We can add a flag that makes netvsc
>behave differently, and if this flag also allows enhanced functionality
>userspace will gradually switch.

Okay, although in this case, it really does not make much sense, so be
it. Leave the netvsc set the ->priv flag to IFF_SLAVE as it is doing
now. (This once-wrong-forever-wrong policy is flustrating me).

But since this patchset introduces private flag IFF_FAILOVER and
IFF_FAILOVER_SLAVE, and we set IFF_FAILOVER to the netvsc netdev
instance, we should also set IFF_FAILOVER_SLAVE to the enslaved VF
netdevice to get at least some consistency between virtio_net and
netvsc.


>
>Anything breaking userspace I fully expect Stephen to nack and
>IMO with good reason.
>
>-- 
>MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v11 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Jiri Pirko]

Tue, May 22, 2018 at 05:32:30PM CEST, m...@redhat.com wrote:
>On Tue, May 22, 2018 at 05:13:43PM +0200, Jiri Pirko wrote:
>> Tue, May 22, 2018 at 03:39:33PM CEST, m...@redhat.com wrote:
>> >On Tue, May 22, 2018 at 03:26:26PM +0200, Jiri Pirko wrote:
>> >> Tue, May 22, 2018 at 03:17:37PM CEST, m...@redhat.com wrote:
>> >> >On Tue, May 22, 2018 at 03:14:22PM +0200, Jiri Pirko wrote:
>> >> >> Tue, May 22, 2018 at 03:12:40PM CEST, m...@redhat.com wrote:
>> >> >> >On Tue, May 22, 2018 at 11:08:53AM +0200, Jiri Pirko wrote:
>> >> >> >> Tue, May 22, 2018 at 11:06:37AM CEST, j...@resnulli.us wrote:
>> >> >> >> >Tue, May 22, 2018 at 04:06:18AM CEST, sridhar.samudr...@intel.com 
>> >> >> >> >wrote:
>> >> >> >> >>Use the registration/notification framework supported by the 
>> >> >> >> >>generic
>> >> >> >> >>failover infrastructure.
>> >> >> >> >>
>> >> >> >> >>Signed-off-by: Sridhar Samudrala 
>> >> >> >> >
>> >> >> >> >In previous patchset versions, the common code did
>> >> >> >> >netdev_rx_handler_register() and netdev_upper_dev_link() etc
>> >> >> >> >(netvsc_vf_join()). Now, this is still done in netvsc. Why?
>> >> >> >> >
>> >> >> >> >This should be part of the common "failover" code.
>> >> >> >> >
>> >> >> >> 
>> >> >> >> Also note that in the current patchset you use IFF_FAILOVER flag for
>> >> >> >> master, yet for the slave you use IFF_SLAVE. That is wrong.
>> >> >> >> IFF_FAILOVER_SLAVE should be used.
>> >> >> >
>> >> >> >Or drop IFF_FAILOVER_SLAVE and set both IFF_FAILOVER and IFF_SLAVE?
>> >> >> 
>> >> >> No. IFF_SLAVE is for bonding.
>> >> >
>> >> >What breaks if we reuse it for failover?
>> >> 
>> >> This is exposed to userspace. IFF_SLAVE is expected for bonding slaves.
>> >> And failover slave is not a bonding slave.
>> >
>> >That does not really answer the question.  I'd claim it's sufficiently
>> >like a bond slave for IFF_SLAVE to make sense.
>> >
>> >In fact you will find that netvsc already sets IFF_SLAVE, and so
>> 
>> netvsc does the whole failover thing in a wrong way. This patchset is
>> trying to fix it.
>
>Maybe, but we don't need gratuitous changes either, especially if they
>break userspace.

What do you mean by the "break"? It was a mistake to reuse IFF_SLAVE at
the first place, lets fix it. If some userspace depends on that flag, it
is broken anyway.


>
>> >does e.g. the eql driver.
>> >
>> >The advantage of using IFF_SLAVE is that userspace knows to skip it.  If
>> 
>> The userspace should know how to skip other types of slaves - team,
>> bridge, ovs, etc.
>> The "master link" should be the one to look at.
>> 
>
>How should existing userspace know which ones to skip and which one is
>the master?  Right now userspace seems to assume whatever does not have
>IFF_SLAVE should be looked at. Are you saying that's not the right thing

Why do you say so? What do you mean by "looked at"? Certainly not.
IFLA_MASTER is the attribute that should be looked at, nothing else.


>to do and userspace should be fixed? What should userspace do in
>your opinion that will be forward compatible with future kernels?
>
>> 
>> >we don't set IFF_SLAVE existing userspace tries to use the lowerdev.
>> 
>> Each master type has a IFF_ master flag and IFF_ slave flag.
>
>Could you give some examples please?

enum netdev_priv_flags {
IFF_EBRIDGE = 1<<1,
IFF_BRIDGE_PORT = 1<<9,
IFF_OPENVSWITCH = 1<<20,
IFF_OVS_DATAPATH= 1<<10,
IFF_L3MDEV_MASTER   = 1<<18,
IFF_L3MDEV_SLAVE= 1<<21,
IFF_TEAM= 1<<22,
IFF_TEAM_PORT   = 1<<13,
};


>
>> In private
>> flag. I don't see no reason to break this pattern here.
>
>Other masters are setup from userspace, this one is set up automatically
>by kernel. So the bar is higher, we need an interface that existing
>userspace knows about.  We can't just say "oh if userspace set this up
>it should know to skip lowerdevs".
>
>Otherwise multiple interfaces with same mac tend to confuse userspace.

No difference, really.
Regardless who does the setup, and independent userspace deamon should
react accordingly.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v11 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Michael S. Tsirkin]

On Tue, May 22, 2018 at 05:13:43PM +0200, Jiri Pirko wrote:
> Tue, May 22, 2018 at 03:39:33PM CEST, m...@redhat.com wrote:
> >On Tue, May 22, 2018 at 03:26:26PM +0200, Jiri Pirko wrote:
> >> Tue, May 22, 2018 at 03:17:37PM CEST, m...@redhat.com wrote:
> >> >On Tue, May 22, 2018 at 03:14:22PM +0200, Jiri Pirko wrote:
> >> >> Tue, May 22, 2018 at 03:12:40PM CEST, m...@redhat.com wrote:
> >> >> >On Tue, May 22, 2018 at 11:08:53AM +0200, Jiri Pirko wrote:
> >> >> >> Tue, May 22, 2018 at 11:06:37AM CEST, j...@resnulli.us wrote:
> >> >> >> >Tue, May 22, 2018 at 04:06:18AM CEST, sridhar.samudr...@intel.com 
> >> >> >> >wrote:
> >> >> >> >>Use the registration/notification framework supported by the 
> >> >> >> >>generic
> >> >> >> >>failover infrastructure.
> >> >> >> >>
> >> >> >> >>Signed-off-by: Sridhar Samudrala 
> >> >> >> >
> >> >> >> >In previous patchset versions, the common code did
> >> >> >> >netdev_rx_handler_register() and netdev_upper_dev_link() etc
> >> >> >> >(netvsc_vf_join()). Now, this is still done in netvsc. Why?
> >> >> >> >
> >> >> >> >This should be part of the common "failover" code.
> >> >> >> >
> >> >> >> 
> >> >> >> Also note that in the current patchset you use IFF_FAILOVER flag for
> >> >> >> master, yet for the slave you use IFF_SLAVE. That is wrong.
> >> >> >> IFF_FAILOVER_SLAVE should be used.
> >> >> >
> >> >> >Or drop IFF_FAILOVER_SLAVE and set both IFF_FAILOVER and IFF_SLAVE?
> >> >> 
> >> >> No. IFF_SLAVE is for bonding.
> >> >
> >> >What breaks if we reuse it for failover?
> >> 
> >> This is exposed to userspace. IFF_SLAVE is expected for bonding slaves.
> >> And failover slave is not a bonding slave.
> >
> >That does not really answer the question.  I'd claim it's sufficiently
> >like a bond slave for IFF_SLAVE to make sense.
> >
> >In fact you will find that netvsc already sets IFF_SLAVE, and so
> 
> netvsc does the whole failover thing in a wrong way. This patchset is
> trying to fix it.

Maybe, but we don't need gratuitous changes either, especially if they
break userspace.

> >does e.g. the eql driver.
> >
> >The advantage of using IFF_SLAVE is that userspace knows to skip it.  If
> 
> The userspace should know how to skip other types of slaves - team,
> bridge, ovs, etc.
> The "master link" should be the one to look at.
> 

How should existing userspace know which ones to skip and which one is
the master?  Right now userspace seems to assume whatever does not have
IFF_SLAVE should be looked at. Are you saying that's not the right thing
to do and userspace should be fixed? What should userspace do in
your opinion that will be forward compatible with future kernels?

> 
> >we don't set IFF_SLAVE existing userspace tries to use the lowerdev.
> 
> Each master type has a IFF_ master flag and IFF_ slave flag.

Could you give some examples please?

> In private
> flag. I don't see no reason to break this pattern here.

Other masters are setup from userspace, this one is set up automatically
by kernel. So the bar is higher, we need an interface that existing
userspace knows about.  We can't just say "oh if userspace set this up
it should know to skip lowerdevs".

Otherwise multiple interfaces with same mac tend to confuse userspace.

-- 
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v11 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Samudrala, Sridhar]

On 5/22/2018 2:08 AM, Jiri Pirko wrote:

Tue, May 22, 2018 at 11:06:37AM CEST, j...@resnulli.us wrote:

Tue, May 22, 2018 at 04:06:18AM CEST, sridhar.samudr...@intel.com wrote:

Use the registration/notification framework supported by the generic
failover infrastructure.

Signed-off-by: Sridhar Samudrala 

In previous patchset versions, the common code did
netdev_rx_handler_register() and netdev_upper_dev_link() etc
(netvsc_vf_join()). Now, this is still done in netvsc. Why?

This should be part of the common "failover" code.


Based on Stephen's feedback on earlier patches, i tried to minimize the changes 
to
netvsc and only commonize the notifier and the main event handler routine.
Another complication is that netvsc does part of registration in a delayed 
workqueue.

It should be possible to move some of the code from net_failover.c to generic
failover.c in future if Stephen is ok with it.





Also note that in the current patchset you use IFF_FAILOVER flag for
master, yet for the slave you use IFF_SLAVE. That is wrong.
IFF_FAILOVER_SLAVE should be used.


Not sure which code you are referring to.  I only set IFF_FAILOVER_SLAVE
in patch 3.

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v11 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Jiri Pirko]

Tue, May 22, 2018 at 03:39:33PM CEST, m...@redhat.com wrote:
>On Tue, May 22, 2018 at 03:26:26PM +0200, Jiri Pirko wrote:
>> Tue, May 22, 2018 at 03:17:37PM CEST, m...@redhat.com wrote:
>> >On Tue, May 22, 2018 at 03:14:22PM +0200, Jiri Pirko wrote:
>> >> Tue, May 22, 2018 at 03:12:40PM CEST, m...@redhat.com wrote:
>> >> >On Tue, May 22, 2018 at 11:08:53AM +0200, Jiri Pirko wrote:
>> >> >> Tue, May 22, 2018 at 11:06:37AM CEST, j...@resnulli.us wrote:
>> >> >> >Tue, May 22, 2018 at 04:06:18AM CEST, sridhar.samudr...@intel.com 
>> >> >> >wrote:
>> >> >> >>Use the registration/notification framework supported by the generic
>> >> >> >>failover infrastructure.
>> >> >> >>
>> >> >> >>Signed-off-by: Sridhar Samudrala 
>> >> >> >
>> >> >> >In previous patchset versions, the common code did
>> >> >> >netdev_rx_handler_register() and netdev_upper_dev_link() etc
>> >> >> >(netvsc_vf_join()). Now, this is still done in netvsc. Why?
>> >> >> >
>> >> >> >This should be part of the common "failover" code.
>> >> >> >
>> >> >> 
>> >> >> Also note that in the current patchset you use IFF_FAILOVER flag for
>> >> >> master, yet for the slave you use IFF_SLAVE. That is wrong.
>> >> >> IFF_FAILOVER_SLAVE should be used.
>> >> >
>> >> >Or drop IFF_FAILOVER_SLAVE and set both IFF_FAILOVER and IFF_SLAVE?
>> >> 
>> >> No. IFF_SLAVE is for bonding.
>> >
>> >What breaks if we reuse it for failover?
>> 
>> This is exposed to userspace. IFF_SLAVE is expected for bonding slaves.
>> And failover slave is not a bonding slave.
>
>That does not really answer the question.  I'd claim it's sufficiently
>like a bond slave for IFF_SLAVE to make sense.
>
>In fact you will find that netvsc already sets IFF_SLAVE, and so

netvsc does the whole failover thing in a wrong way. This patchset is
trying to fix it.

>does e.g. the eql driver.
>
>The advantage of using IFF_SLAVE is that userspace knows to skip it.  If

The userspace should know how to skip other types of slaves - team,
bridge, ovs, etc. The "master link" should be the one to look at.


>we don't set IFF_SLAVE existing userspace tries to use the lowerdev.

Each master type has a IFF_ master flag and IFF_ slave flag. In private
flag. I don't see no reason to break this pattern here.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v11 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Michael S. Tsirkin]

On Tue, May 22, 2018 at 03:26:26PM +0200, Jiri Pirko wrote:
> Tue, May 22, 2018 at 03:17:37PM CEST, m...@redhat.com wrote:
> >On Tue, May 22, 2018 at 03:14:22PM +0200, Jiri Pirko wrote:
> >> Tue, May 22, 2018 at 03:12:40PM CEST, m...@redhat.com wrote:
> >> >On Tue, May 22, 2018 at 11:08:53AM +0200, Jiri Pirko wrote:
> >> >> Tue, May 22, 2018 at 11:06:37AM CEST, j...@resnulli.us wrote:
> >> >> >Tue, May 22, 2018 at 04:06:18AM CEST, sridhar.samudr...@intel.com 
> >> >> >wrote:
> >> >> >>Use the registration/notification framework supported by the generic
> >> >> >>failover infrastructure.
> >> >> >>
> >> >> >>Signed-off-by: Sridhar Samudrala 
> >> >> >
> >> >> >In previous patchset versions, the common code did
> >> >> >netdev_rx_handler_register() and netdev_upper_dev_link() etc
> >> >> >(netvsc_vf_join()). Now, this is still done in netvsc. Why?
> >> >> >
> >> >> >This should be part of the common "failover" code.
> >> >> >
> >> >> 
> >> >> Also note that in the current patchset you use IFF_FAILOVER flag for
> >> >> master, yet for the slave you use IFF_SLAVE. That is wrong.
> >> >> IFF_FAILOVER_SLAVE should be used.
> >> >
> >> >Or drop IFF_FAILOVER_SLAVE and set both IFF_FAILOVER and IFF_SLAVE?
> >> 
> >> No. IFF_SLAVE is for bonding.
> >
> >What breaks if we reuse it for failover?
> 
> This is exposed to userspace. IFF_SLAVE is expected for bonding slaves.
> And failover slave is not a bonding slave.

That does not really answer the question.  I'd claim it's sufficiently
like a bond slave for IFF_SLAVE to make sense.

In fact you will find that netvsc already sets IFF_SLAVE, and so
does e.g. the eql driver.

The advantage of using IFF_SLAVE is that userspace knows to skip it.  If
we don't set IFF_SLAVE existing userspace tries to use the lowerdev.

-- 
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v11 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Jiri Pirko]

Tue, May 22, 2018 at 03:17:37PM CEST, m...@redhat.com wrote:
>On Tue, May 22, 2018 at 03:14:22PM +0200, Jiri Pirko wrote:
>> Tue, May 22, 2018 at 03:12:40PM CEST, m...@redhat.com wrote:
>> >On Tue, May 22, 2018 at 11:08:53AM +0200, Jiri Pirko wrote:
>> >> Tue, May 22, 2018 at 11:06:37AM CEST, j...@resnulli.us wrote:
>> >> >Tue, May 22, 2018 at 04:06:18AM CEST, sridhar.samudr...@intel.com wrote:
>> >> >>Use the registration/notification framework supported by the generic
>> >> >>failover infrastructure.
>> >> >>
>> >> >>Signed-off-by: Sridhar Samudrala 
>> >> >
>> >> >In previous patchset versions, the common code did
>> >> >netdev_rx_handler_register() and netdev_upper_dev_link() etc
>> >> >(netvsc_vf_join()). Now, this is still done in netvsc. Why?
>> >> >
>> >> >This should be part of the common "failover" code.
>> >> >
>> >> 
>> >> Also note that in the current patchset you use IFF_FAILOVER flag for
>> >> master, yet for the slave you use IFF_SLAVE. That is wrong.
>> >> IFF_FAILOVER_SLAVE should be used.
>> >
>> >Or drop IFF_FAILOVER_SLAVE and set both IFF_FAILOVER and IFF_SLAVE?
>> 
>> No. IFF_SLAVE is for bonding.
>
>What breaks if we reuse it for failover?

This is exposed to userspace. IFF_SLAVE is expected for bonding slaves.
And failover slave is not a bonding slave.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v11 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Michael S. Tsirkin]

On Tue, May 22, 2018 at 03:14:22PM +0200, Jiri Pirko wrote:
> Tue, May 22, 2018 at 03:12:40PM CEST, m...@redhat.com wrote:
> >On Tue, May 22, 2018 at 11:08:53AM +0200, Jiri Pirko wrote:
> >> Tue, May 22, 2018 at 11:06:37AM CEST, j...@resnulli.us wrote:
> >> >Tue, May 22, 2018 at 04:06:18AM CEST, sridhar.samudr...@intel.com wrote:
> >> >>Use the registration/notification framework supported by the generic
> >> >>failover infrastructure.
> >> >>
> >> >>Signed-off-by: Sridhar Samudrala 
> >> >
> >> >In previous patchset versions, the common code did
> >> >netdev_rx_handler_register() and netdev_upper_dev_link() etc
> >> >(netvsc_vf_join()). Now, this is still done in netvsc. Why?
> >> >
> >> >This should be part of the common "failover" code.
> >> >
> >> 
> >> Also note that in the current patchset you use IFF_FAILOVER flag for
> >> master, yet for the slave you use IFF_SLAVE. That is wrong.
> >> IFF_FAILOVER_SLAVE should be used.
> >
> >Or drop IFF_FAILOVER_SLAVE and set both IFF_FAILOVER and IFF_SLAVE?
> 
> No. IFF_SLAVE is for bonding.

What breaks if we reuse it for failover?

-- 
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v11 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Jiri Pirko]

Tue, May 22, 2018 at 03:12:40PM CEST, m...@redhat.com wrote:
>On Tue, May 22, 2018 at 11:08:53AM +0200, Jiri Pirko wrote:
>> Tue, May 22, 2018 at 11:06:37AM CEST, j...@resnulli.us wrote:
>> >Tue, May 22, 2018 at 04:06:18AM CEST, sridhar.samudr...@intel.com wrote:
>> >>Use the registration/notification framework supported by the generic
>> >>failover infrastructure.
>> >>
>> >>Signed-off-by: Sridhar Samudrala 
>> >
>> >In previous patchset versions, the common code did
>> >netdev_rx_handler_register() and netdev_upper_dev_link() etc
>> >(netvsc_vf_join()). Now, this is still done in netvsc. Why?
>> >
>> >This should be part of the common "failover" code.
>> >
>> 
>> Also note that in the current patchset you use IFF_FAILOVER flag for
>> master, yet for the slave you use IFF_SLAVE. That is wrong.
>> IFF_FAILOVER_SLAVE should be used.
>
>Or drop IFF_FAILOVER_SLAVE and set both IFF_FAILOVER and IFF_SLAVE?

No. IFF_SLAVE is for bonding.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v11 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Michael S. Tsirkin]

On Tue, May 22, 2018 at 11:08:53AM +0200, Jiri Pirko wrote:
> Tue, May 22, 2018 at 11:06:37AM CEST, j...@resnulli.us wrote:
> >Tue, May 22, 2018 at 04:06:18AM CEST, sridhar.samudr...@intel.com wrote:
> >>Use the registration/notification framework supported by the generic
> >>failover infrastructure.
> >>
> >>Signed-off-by: Sridhar Samudrala 
> >
> >In previous patchset versions, the common code did
> >netdev_rx_handler_register() and netdev_upper_dev_link() etc
> >(netvsc_vf_join()). Now, this is still done in netvsc. Why?
> >
> >This should be part of the common "failover" code.
> >
> 
> Also note that in the current patchset you use IFF_FAILOVER flag for
> master, yet for the slave you use IFF_SLAVE. That is wrong.
> IFF_FAILOVER_SLAVE should be used.

Or drop IFF_FAILOVER_SLAVE and set both IFF_FAILOVER and IFF_SLAVE?

-- 
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v11 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Jiri Pirko]

Tue, May 22, 2018 at 11:06:37AM CEST, j...@resnulli.us wrote:
>Tue, May 22, 2018 at 04:06:18AM CEST, sridhar.samudr...@intel.com wrote:
>>Use the registration/notification framework supported by the generic
>>failover infrastructure.
>>
>>Signed-off-by: Sridhar Samudrala 
>
>In previous patchset versions, the common code did
>netdev_rx_handler_register() and netdev_upper_dev_link() etc
>(netvsc_vf_join()). Now, this is still done in netvsc. Why?
>
>This should be part of the common "failover" code.
>

Also note that in the current patchset you use IFF_FAILOVER flag for
master, yet for the slave you use IFF_SLAVE. That is wrong.
IFF_FAILOVER_SLAVE should be used.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v11 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Jiri Pirko]

Tue, May 22, 2018 at 04:06:18AM CEST, sridhar.samudr...@intel.com wrote:
>Use the registration/notification framework supported by the generic
>failover infrastructure.
>
>Signed-off-by: Sridhar Samudrala 

In previous patchset versions, the common code did
netdev_rx_handler_register() and netdev_upper_dev_link() etc
(netvsc_vf_join()). Now, this is still done in netvsc. Why?

This should be part of the common "failover" code.

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization