From: "Samudrala, Sridhar" <sridhar.samudrala@intel.com> To: Jiri Pirko <jiri@resnulli.us>, "Michael S. Tsirkin" <mst@redhat.com> Cc: stephen@networkplumber.org, davem@davemloft.net, netdev@vger.kernel.org, virtualization@lists.linux-foundation.org, virtio-dev@lists.oasis-open.org, jesse.brandeburg@intel.com, alexander.h.duyck@intel.com, kubakici@wp.pl, jasowang@redhat.com, loseweigh@gmail.com Subject: Re: [RFC PATCH net-next v6 2/4] net: Introduce generic bypass module Date: Wed, 18 Apr 2018 15:46:11 -0700 [thread overview] Message-ID: <ff0c5ea1-16d4-00a7-9952-9049efa818eb@intel.com> (raw) In-Reply-To: <20180418203206.GC1922@nanopsycho> On 4/18/2018 1:32 PM, Jiri Pirko wrote: >>>>>>> You still use "active"/"backup" names which is highly misleading as >>>>>>> it has completely different meaning that in bond for example. >>>>>>> I noted that in my previous review already. Please change it. >>>>>> I guess the issue is with only the 'active' name. 'backup' should be fine as it also >>>>>> matches with the BACKUP feature bit we are adding to virtio_net. >>>>> I think that "backup" is also misleading. Both "active" and "backup" >>>>> mean a *state* of slaves. This should be named differently. >>>>> >>>>> >>>>> >>>>>> With regards to alternate names for 'active', you suggested 'stolen', but i >>>>>> am not too happy with it. >>>>>> netvsc uses vf_netdev, are you OK with this? Or another option is 'passthru' >>>>> No. The netdev could be any netdevice. It does not have to be a "VF". >>>>> I think "stolen" is quite appropriate since it describes the modus >>>>> operandi. The bypass master steals some netdevice according to some >>>>> match. >>>>> >>>>> But I don't insist on "stolen". Just sounds right. >>>> We are adding VIRTIO_NET_F_BACKUP as a new feature bit to enable this feature, So i think >>>> 'backup' name is consistent. >>> It perhaps makes sense from the view of virtio device. However, as I >>> described couple of times, for master/slave device the name "backup" is >>> highly misleading. >> virtio is the backup. You are supposed to use another >> (typically passthrough) device, if that fails use virtio. >> It does seem appropriate to me. If you like, we can >> change that to "standby". Active I don't like either. "main"? > Sounds much better, yes. OK. Will change backup to 'standby'. 'main' is fine, what about 'primary'? > > >> In fact would failover be better than bypass? > Also, much better. So do we want to change all 'bypass' references to 'failover' including the filenames.(net/core/failover.c and include/net/failover.h) <snip> > > >> >>>> The intent is to restrict the 'active' netdev to be a VF. If there is a way to check that >>>> a PCI device is a VF in the guest kernel, we could restrict 'active' netdev to be a VF. >>>> >>>> Will look for any suggestions in the next day or two. If i don't get any, i will go >>>> with 'stolen' >>>> >>>> <snip> >>>> >>>> >>>>> + >>>>> +static struct net_device *bypass_master_get_bymac(u8 *mac, >>>>> + struct bypass_ops **ops) >>>>> +{ >>>>> + struct bypass_master *bypass_master; >>>>> + struct net_device *bypass_netdev; >>>>> + >>>>> + spin_lock(&bypass_lock); >>>>> + list_for_each_entry(bypass_master, &bypass_master_list, list) { >>>>>>> As I wrote the last time, you don't need this list, spinlock. >>>>>>> You can do just something like: >>>>>>> for_each_net(net) { >>>>>>> for_each_netdev(net, dev) { >>>>>>> if (netif_is_bypass_master(dev)) { >>>>>> This function returns the upper netdev as well as the ops associated >>>>>> with that netdev. >>>>>> bypass_master_list is a list of 'struct bypass_master' that associates >>>>> Well, can't you have it in netdev priv? >>>> We cannot do this for 2-netdev model as there is no bypass_netdev created. >>> Howcome? You have no master? I don't understand.. For 2-netdev model, the master netdev is not a new one created by the bypass module. It is created by netvsc internally and passed via bypass_master_register() <snip> >>> >>>>>>>> + >>>>>>>> + /* Avoid Bonding master dev with same MAC registering as slave dev */ >>>>>>>> + if ((dev->priv_flags & IFF_BONDING) && (dev->flags & IFF_MASTER)) >>>>>>> Yeah, this is certainly incorrect. One thing is, you should be using the >>>>>>> helpers netif_is_bond_master(). >>>>>>> But what about the rest? macsec, macvlan, team, bridge, ovs and others? >>>>>>> >>>>>>> You need to do it not by blacklisting, but with whitelisting. You need >>>>>>> to whitelist VF devices. My port flavours patchset might help with this. >>>>>> May be i can use netdev_has_lower_dev() helper to make sure that the slave >>>>> I don't see such function in the code. >>>> It is netdev_has_any_lower_dev(). I need to export it. >>> Come on, you cannot use that. That would allow bonding without slaves, >>> but the slaves could be added later on. >>> >>> What exactly you are trying to achieve by this? I think i can remove this check. In pre-register, for backup device, i check that its parent matches bypass device & for vf device, we make sure that it is a pci device.
WARNING: multiple messages have this Message-ID (diff)
From: "Samudrala, Sridhar" <sridhar.samudrala@intel.com> To: Jiri Pirko <jiri@resnulli.us>, "Michael S. Tsirkin" <mst@redhat.com> Cc: stephen@networkplumber.org, davem@davemloft.net, netdev@vger.kernel.org, virtualization@lists.linux-foundation.org, virtio-dev@lists.oasis-open.org, jesse.brandeburg@intel.com, alexander.h.duyck@intel.com, kubakici@wp.pl, jasowang@redhat.com, loseweigh@gmail.com Subject: [virtio-dev] Re: [RFC PATCH net-next v6 2/4] net: Introduce generic bypass module Date: Wed, 18 Apr 2018 15:46:11 -0700 [thread overview] Message-ID: <ff0c5ea1-16d4-00a7-9952-9049efa818eb@intel.com> (raw) In-Reply-To: <20180418203206.GC1922@nanopsycho> On 4/18/2018 1:32 PM, Jiri Pirko wrote: >>>>>>> You still use "active"/"backup" names which is highly misleading as >>>>>>> it has completely different meaning that in bond for example. >>>>>>> I noted that in my previous review already. Please change it. >>>>>> I guess the issue is with only the 'active' name. 'backup' should be fine as it also >>>>>> matches with the BACKUP feature bit we are adding to virtio_net. >>>>> I think that "backup" is also misleading. Both "active" and "backup" >>>>> mean a *state* of slaves. This should be named differently. >>>>> >>>>> >>>>> >>>>>> With regards to alternate names for 'active', you suggested 'stolen', but i >>>>>> am not too happy with it. >>>>>> netvsc uses vf_netdev, are you OK with this? Or another option is 'passthru' >>>>> No. The netdev could be any netdevice. It does not have to be a "VF". >>>>> I think "stolen" is quite appropriate since it describes the modus >>>>> operandi. The bypass master steals some netdevice according to some >>>>> match. >>>>> >>>>> But I don't insist on "stolen". Just sounds right. >>>> We are adding VIRTIO_NET_F_BACKUP as a new feature bit to enable this feature, So i think >>>> 'backup' name is consistent. >>> It perhaps makes sense from the view of virtio device. However, as I >>> described couple of times, for master/slave device the name "backup" is >>> highly misleading. >> virtio is the backup. You are supposed to use another >> (typically passthrough) device, if that fails use virtio. >> It does seem appropriate to me. If you like, we can >> change that to "standby". Active I don't like either. "main"? > Sounds much better, yes. OK. Will change backup to 'standby'. 'main' is fine, what about 'primary'? > > >> In fact would failover be better than bypass? > Also, much better. So do we want to change all 'bypass' references to 'failover' including the filenames.(net/core/failover.c and include/net/failover.h) <snip> > > >> >>>> The intent is to restrict the 'active' netdev to be a VF. If there is a way to check that >>>> a PCI device is a VF in the guest kernel, we could restrict 'active' netdev to be a VF. >>>> >>>> Will look for any suggestions in the next day or two. If i don't get any, i will go >>>> with 'stolen' >>>> >>>> <snip> >>>> >>>> >>>>> + >>>>> +static struct net_device *bypass_master_get_bymac(u8 *mac, >>>>> + struct bypass_ops **ops) >>>>> +{ >>>>> + struct bypass_master *bypass_master; >>>>> + struct net_device *bypass_netdev; >>>>> + >>>>> + spin_lock(&bypass_lock); >>>>> + list_for_each_entry(bypass_master, &bypass_master_list, list) { >>>>>>> As I wrote the last time, you don't need this list, spinlock. >>>>>>> You can do just something like: >>>>>>> for_each_net(net) { >>>>>>> for_each_netdev(net, dev) { >>>>>>> if (netif_is_bypass_master(dev)) { >>>>>> This function returns the upper netdev as well as the ops associated >>>>>> with that netdev. >>>>>> bypass_master_list is a list of 'struct bypass_master' that associates >>>>> Well, can't you have it in netdev priv? >>>> We cannot do this for 2-netdev model as there is no bypass_netdev created. >>> Howcome? You have no master? I don't understand.. For 2-netdev model, the master netdev is not a new one created by the bypass module. It is created by netvsc internally and passed via bypass_master_register() <snip> >>> >>>>>>>> + >>>>>>>> + /* Avoid Bonding master dev with same MAC registering as slave dev */ >>>>>>>> + if ((dev->priv_flags & IFF_BONDING) && (dev->flags & IFF_MASTER)) >>>>>>> Yeah, this is certainly incorrect. One thing is, you should be using the >>>>>>> helpers netif_is_bond_master(). >>>>>>> But what about the rest? macsec, macvlan, team, bridge, ovs and others? >>>>>>> >>>>>>> You need to do it not by blacklisting, but with whitelisting. You need >>>>>>> to whitelist VF devices. My port flavours patchset might help with this. >>>>>> May be i can use netdev_has_lower_dev() helper to make sure that the slave >>>>> I don't see such function in the code. >>>> It is netdev_has_any_lower_dev(). I need to export it. >>> Come on, you cannot use that. That would allow bonding without slaves, >>> but the slaves could be added later on. >>> >>> What exactly you are trying to achieve by this? I think i can remove this check. In pre-register, for backup device, i check that its parent matches bypass device & for vf device, we make sure that it is a pci device. --------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
next prev parent reply other threads:[~2018-04-18 22:46 UTC|newest] Thread overview: 147+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-04-10 18:59 [RFC PATCH net-next v6 0/4] Enable virtio_net to act as a backup for a passthru device Sridhar Samudrala 2018-04-10 18:59 ` [virtio-dev] " Sridhar Samudrala 2018-04-10 18:59 ` [RFC PATCH net-next v6 1/4] virtio_net: Introduce VIRTIO_NET_F_BACKUP feature bit Sridhar Samudrala 2018-04-10 18:59 ` [virtio-dev] " Sridhar Samudrala 2018-04-10 18:59 ` [RFC PATCH net-next v6 2/4] net: Introduce generic bypass module Sridhar Samudrala 2018-04-10 18:59 ` [virtio-dev] " Sridhar Samudrala 2018-04-11 15:51 ` Jiri Pirko 2018-04-11 19:13 ` Samudrala, Sridhar 2018-04-11 19:13 ` Samudrala, Sridhar 2018-04-11 19:13 ` [virtio-dev] " Samudrala, Sridhar 2018-04-18 9:25 ` Jiri Pirko 2018-04-18 9:25 ` Jiri Pirko 2018-04-18 18:43 ` Samudrala, Sridhar 2018-04-18 18:43 ` Samudrala, Sridhar 2018-04-18 18:43 ` [virtio-dev] " Samudrala, Sridhar 2018-04-18 19:13 ` Jiri Pirko 2018-04-18 19:13 ` Jiri Pirko 2018-04-18 19:46 ` Michael S. Tsirkin 2018-04-18 19:46 ` [virtio-dev] " Michael S. Tsirkin 2018-04-18 20:32 ` Jiri Pirko 2018-04-18 22:46 ` Samudrala, Sridhar [this message] 2018-04-18 22:46 ` [virtio-dev] " Samudrala, Sridhar 2018-04-19 6:35 ` Jiri Pirko 2018-04-19 6:35 ` Jiri Pirko 2018-04-18 22:46 ` Samudrala, Sridhar 2018-04-19 4:08 ` Michael S. Tsirkin 2018-04-19 4:08 ` [virtio-dev] " Michael S. Tsirkin 2018-04-19 7:22 ` Jiri Pirko 2018-04-19 7:22 ` Jiri Pirko 2018-04-19 4:08 ` Michael S. Tsirkin 2018-04-18 20:32 ` Jiri Pirko 2018-04-11 15:51 ` Jiri Pirko 2018-04-10 18:59 ` [RFC PATCH net-next v6 3/4] virtio_net: Extend virtio to use VF datapath when available Sridhar Samudrala 2018-04-10 18:59 ` [virtio-dev] " Sridhar Samudrala 2018-04-10 18:59 ` [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework Sridhar Samudrala 2018-04-10 18:59 ` [virtio-dev] " Sridhar Samudrala 2018-04-10 21:26 ` Stephen Hemminger 2018-04-10 22:56 ` Samudrala, Sridhar 2018-04-10 22:56 ` [virtio-dev] " Samudrala, Sridhar 2018-04-10 23:28 ` Michael S. Tsirkin 2018-04-10 23:28 ` Michael S. Tsirkin 2018-04-10 23:28 ` [virtio-dev] " Michael S. Tsirkin 2018-04-10 23:44 ` Siwei Liu 2018-04-10 23:44 ` [virtio-dev] " Siwei Liu 2018-04-10 23:59 ` Stephen Hemminger 2018-04-10 23:44 ` Siwei Liu 2018-04-11 7:50 ` Jiri Pirko 2018-04-11 7:50 ` Jiri Pirko 2018-04-11 1:21 ` Michael S. Tsirkin 2018-04-11 1:21 ` Michael S. Tsirkin 2018-04-11 7:53 ` Jiri Pirko 2018-04-11 7:53 ` Jiri Pirko 2019-02-22 1:14 ` net_failover slave udev renaming (was Re: [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework) Siwei Liu 2019-02-22 1:14 ` [virtio-dev] " Siwei Liu 2019-02-22 1:39 ` Michael S. Tsirkin 2019-02-22 1:39 ` [virtio-dev] " Michael S. Tsirkin 2019-02-22 3:33 ` si-wei liu 2019-02-22 3:33 ` si-wei liu 2019-02-22 7:00 ` Samudrala, Sridhar 2019-02-22 7:55 ` si-wei liu 2019-02-22 7:55 ` si-wei liu 2019-02-22 12:58 ` Rob Miller 2019-02-22 12:58 ` Rob Miller 2019-02-22 15:14 ` Michael S. Tsirkin 2019-02-22 15:14 ` Michael S. Tsirkin 2019-02-26 0:58 ` si-wei liu 2019-02-26 0:58 ` si-wei liu 2019-02-26 1:39 ` Stephen Hemminger 2019-02-26 1:39 ` Stephen Hemminger 2019-02-26 2:05 ` Michael S. Tsirkin 2019-02-26 2:05 ` Michael S. Tsirkin 2019-02-26 2:05 ` Michael S. Tsirkin 2019-02-27 0:49 ` si-wei liu 2019-02-27 0:49 ` si-wei liu 2019-02-26 2:08 ` Michael S. Tsirkin 2019-02-26 2:08 ` Michael S. Tsirkin 2019-02-26 2:08 ` Michael S. Tsirkin 2019-02-27 0:17 ` si-wei liu 2019-02-27 0:17 ` si-wei liu 2019-02-27 21:57 ` Stephen Hemminger 2019-02-27 21:57 ` Stephen Hemminger 2019-02-27 22:30 ` si-wei liu 2019-02-27 22:30 ` si-wei liu 2019-02-27 22:38 ` Michael S. Tsirkin 2019-02-27 22:38 ` Michael S. Tsirkin 2019-02-27 22:38 ` Michael S. Tsirkin 2019-02-27 23:34 ` si-wei liu 2019-02-27 23:34 ` si-wei liu 2019-02-27 23:50 ` Michael S. Tsirkin 2019-02-27 23:50 ` Michael S. Tsirkin 2019-02-27 23:50 ` Michael S. Tsirkin 2019-02-28 0:00 ` Liran Alon 2019-02-28 0:00 ` Liran Alon 2019-02-28 0:03 ` Stephen Hemminger 2019-02-28 0:38 ` Michael S. Tsirkin 2019-02-28 0:38 ` Michael S. Tsirkin 2019-02-28 0:38 ` Michael S. Tsirkin 2019-02-28 0:03 ` Stephen Hemminger 2019-02-28 0:38 ` si-wei liu 2019-02-28 0:38 ` si-wei liu 2019-02-28 0:41 ` Michael S. Tsirkin 2019-02-28 0:41 ` Michael S. Tsirkin 2019-02-28 0:41 ` Michael S. Tsirkin 2019-02-28 0:52 ` Jakub Kicinski 2019-02-28 0:52 ` Jakub Kicinski 2019-02-28 1:26 ` Michael S. Tsirkin 2019-02-28 1:26 ` Michael S. Tsirkin 2019-02-28 1:52 ` Jakub Kicinski 2019-02-28 1:52 ` Jakub Kicinski 2019-02-28 4:47 ` Michael S. Tsirkin 2019-02-28 4:47 ` Michael S. Tsirkin 2019-02-28 4:47 ` Michael S. Tsirkin 2019-02-28 18:13 ` Jakub Kicinski 2019-02-28 19:36 ` Michael S. Tsirkin 2019-02-28 19:36 ` Michael S. Tsirkin 2019-02-28 19:36 ` Michael S. Tsirkin 2019-02-28 19:56 ` Jakub Kicinski 2019-02-28 19:56 ` Jakub Kicinski 2019-02-28 20:14 ` Michael S. Tsirkin 2019-02-28 20:14 ` Michael S. Tsirkin 2019-02-28 23:31 ` Jakub Kicinski 2019-02-28 23:31 ` Jakub Kicinski 2019-03-01 0:20 ` Siwei Liu 2019-03-01 0:20 ` Siwei Liu 2019-03-01 1:05 ` Jakub Kicinski 2019-03-02 0:30 ` Siwei Liu 2019-03-02 0:30 ` Siwei Liu 2019-03-01 1:05 ` Jakub Kicinski 2019-02-28 18:13 ` Jakub Kicinski 2019-02-28 1:26 ` Michael S. Tsirkin 2019-02-28 9:32 ` si-wei liu 2019-02-28 9:32 ` si-wei liu 2019-02-28 14:26 ` Michael S. Tsirkin 2019-02-28 14:26 ` Michael S. Tsirkin 2019-03-01 1:30 ` si-wei liu 2019-03-01 1:30 ` si-wei liu 2019-03-01 13:27 ` Michael S. Tsirkin 2019-03-01 13:27 ` Michael S. Tsirkin 2019-03-01 13:27 ` Michael S. Tsirkin 2019-03-01 20:55 ` si-wei liu 2019-03-01 20:55 ` si-wei liu 2019-02-28 14:26 ` Michael S. Tsirkin 2019-02-22 15:14 ` Michael S. Tsirkin 2019-02-22 7:00 ` Samudrala, Sridhar 2019-02-22 1:39 ` Michael S. Tsirkin 2019-02-22 1:14 ` Siwei Liu 2018-04-10 21:26 ` [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework Stephen Hemminger
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=ff0c5ea1-16d4-00a7-9952-9049efa818eb@intel.com \ --to=sridhar.samudrala@intel.com \ --cc=alexander.h.duyck@intel.com \ --cc=davem@davemloft.net \ --cc=jasowang@redhat.com \ --cc=jesse.brandeburg@intel.com \ --cc=jiri@resnulli.us \ --cc=kubakici@wp.pl \ --cc=loseweigh@gmail.com \ --cc=mst@redhat.com \ --cc=netdev@vger.kernel.org \ --cc=stephen@networkplumber.org \ --cc=virtio-dev@lists.oasis-open.org \ --cc=virtualization@lists.linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.