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.
next prev parent reply other threads:[~2018-04-18 22:46 UTC|newest]
Thread overview: 63+ 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 ` [RFC PATCH net-next v6 1/4] virtio_net: Introduce VIRTIO_NET_F_BACKUP feature bit Sridhar Samudrala
2018-04-10 18:59 ` [RFC PATCH net-next v6 2/4] net: Introduce generic bypass module Sridhar Samudrala
2018-04-11 15:51 ` Jiri Pirko
2018-04-11 19:13 ` Samudrala, Sridhar
2018-04-18 9:25 ` Jiri Pirko
2018-04-18 18:43 ` Samudrala, Sridhar
2018-04-18 19:13 ` Jiri Pirko
2018-04-18 19:46 ` Michael S. Tsirkin
2018-04-18 20:32 ` Jiri Pirko
2018-04-18 22:46 ` Samudrala, Sridhar [this message]
2018-04-19 6:35 ` Jiri Pirko
2018-04-19 4:08 ` Michael S. Tsirkin
2018-04-19 7:22 ` 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 ` [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework Sridhar Samudrala
2018-04-10 21:26 ` Stephen Hemminger
2018-04-10 22:56 ` Samudrala, Sridhar
2018-04-10 23:28 ` Michael S. Tsirkin
2018-04-10 23:44 ` Siwei Liu
2018-04-10 23:59 ` Stephen Hemminger
2018-04-11 7:50 ` Jiri Pirko
2018-04-11 1:21 ` Michael S. Tsirkin
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:39 ` Michael S. Tsirkin
2019-02-22 3:33 ` [virtio-dev] " si-wei liu
[not found] ` <91d4cbb1-be7a-b53c-6b2a-99bef07e7c53@intel.com>
2019-02-22 7:55 ` si-wei liu
2019-02-22 12:58 ` Rob Miller
2019-02-22 15:14 ` Michael S. Tsirkin
2019-02-26 0:58 ` si-wei liu
2019-02-26 1:39 ` Stephen Hemminger
2019-02-26 2:05 ` Michael S. Tsirkin
2019-02-27 0:49 ` si-wei liu
2019-02-26 2:08 ` Michael S. Tsirkin
2019-02-27 0:17 ` si-wei liu
2019-02-27 21:57 ` Stephen Hemminger
2019-02-27 22:30 ` si-wei liu
2019-02-27 22:38 ` Michael S. Tsirkin
2019-02-27 23:34 ` si-wei liu
2019-02-27 23:50 ` Michael S. Tsirkin
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 ` si-wei liu
2019-02-28 0:41 ` Michael S. Tsirkin
2019-02-28 0:52 ` Jakub Kicinski
2019-02-28 1:26 ` Michael S. Tsirkin
2019-02-28 1:52 ` Jakub Kicinski
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:56 ` Jakub Kicinski
2019-02-28 20:14 ` Michael S. Tsirkin
2019-02-28 23:31 ` Jakub Kicinski
2019-03-01 0:20 ` Siwei Liu
2019-03-01 1:05 ` Jakub Kicinski
2019-03-02 0:30 ` Siwei Liu
2019-02-28 9:32 ` si-wei liu
2019-02-28 14:26 ` Michael S. Tsirkin
2019-03-01 1:30 ` si-wei liu
2019-03-01 13:27 ` Michael S. Tsirkin
2019-03-01 20:55 ` si-wei liu
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$(echo .)com \
--cc=alexander.h.duyck@intel$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=jasowang@redhat$(echo .)com \
--cc=jesse.brandeburg@intel$(echo .)com \
--cc=jiri@resnulli$(echo .)us \
--cc=kubakici@wp$(echo .)pl \
--cc=loseweigh@gmail$(echo .)com \
--cc=mst@redhat$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=stephen@networkplumber$(echo .)org \
--cc=virtio-dev@lists$(echo .)oasis-open.org \
--cc=virtualization@lists$(echo .)linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox