* Re: [RFC PATCH v3 0/3] Enable virtio_net to act as a backup for a passthru device
From: Jiri Pirko @ 2018-02-21 16:58 UTC (permalink / raw)
To: Alexander Duyck
Cc: Duyck, Alexander H, virtio-dev, Michael S. Tsirkin,
Jakub Kicinski, Samudrala, Sridhar, virtualization, Siwei Liu,
Netdev, David Miller
In-Reply-To: <CAKgT0Ud+6eiY7ycyju1SToeatH1R7KBDcUOqjn9Pu2k5uOr5Wg@mail.gmail.com>
Wed, Feb 21, 2018 at 05:49:49PM CET, alexander.duyck@gmail.com wrote:
>On Wed, Feb 21, 2018 at 8:11 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Wed, Feb 21, 2018 at 04:56:48PM CET, alexander.duyck@gmail.com wrote:
>>>On Wed, Feb 21, 2018 at 1:51 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>> Tue, Feb 20, 2018 at 11:33:56PM CET, kubakici@wp.pl wrote:
>>>>>On Tue, 20 Feb 2018 21:14:10 +0100, Jiri Pirko wrote:
>>>>>> Yeah, I can see it now :( I guess that the ship has sailed and we are
>>>>>> stuck with this ugly thing forever...
>>>>>>
>>>>>> Could you at least make some common code that is shared in between
>>>>>> netvsc and virtio_net so this is handled in exacly the same way in both?
>>>>>
>>>>>IMHO netvsc is a vendor specific driver which made a mistake on what
>>>>>behaviour it provides (or tried to align itself with Windows SR-IOV).
>>>>>Let's not make a far, far more commonly deployed and important driver
>>>>>(virtio) bug-compatible with netvsc.
>>>>
>>>> Yeah. netvsc solution is a dangerous precedent here and in my opinition
>>>> it was a huge mistake to merge it. I personally would vote to unmerge it
>>>> and make the solution based on team/bond.
>>>>
>>>>
>>>>>
>>>>>To Jiri's initial comments, I feel the same way, in fact I've talked to
>>>>>the NetworkManager guys to get auto-bonding based on MACs handled in
>>>>>user space. I think it may very well get done in next versions of NM,
>>>>>but isn't done yet. Stephen also raised the point that not everybody is
>>>>>using NM.
>>>>
>>>> Can be done in NM, networkd or other network management tools.
>>>> Even easier to do this in teamd and let them all benefit.
>>>>
>>>> Actually, I took a stab to implement this in teamd. Took me like an hour
>>>> and half.
>>>>
>>>> You can just run teamd with config option "kidnap" like this:
>>>> # teamd/teamd -c '{"kidnap": true }'
>>>>
>>>> Whenever teamd sees another netdev to appear with the same mac as his,
>>>> or whenever teamd sees another netdev to change mac to his,
>>>> it enslaves it.
>>>>
>>>> Here's the patch (quick and dirty):
>>>>
>>>> Subject: [patch teamd] teamd: introduce kidnap feature
>>>>
>>>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>>>
>>>So this doesn't really address the original problem we were trying to
>>>solve. You asked earlier why the netdev name mattered and it mostly
>>>has to do with configuration. Specifically what our patch is
>>>attempting to resolve is the issue of how to allow a cloud provider to
>>>upgrade their customer to SR-IOV support and live migration without
>>>requiring them to reconfigure their guest. So the general idea with
>>>our patch is to take a VM that is running with virtio_net only and
>>>allow it to instead spawn a virtio_bypass master using the same netdev
>>>name as the original virtio, and then have the virtio_net and VF come
>>>up and be enslaved by the bypass interface. Doing it this way we can
>>>allow for multi-vendor SR-IOV live migration support using a guest
>>>that was originally configured for virtio only.
>>>
>>>The problem with your solution is we already have teaming and bonding
>>>as you said. There is already a write-up from Red Hat on how to do it
>>>(https://access.redhat.com/documentation/en-us/red_hat_virtualization/4.1/html/virtual_machine_management_guide/sect-migrating_virtual_machines_between_hosts).
>>>That is all well and good as long as you are willing to keep around
>>>two VM images, one for virtio, and one for SR-IOV with live migration.
>>
>> You don't need 2 images. You need only one. The one with the team setup.
>> That's it. If another netdev with the same mac appears, teamd will
>> enslave it and run traffic on it. If not, ok, you'll go only through
>> virtio_net.
>
>Isn't that going to cause the routing table to get messed up when we
>rearrange the netdevs? We don't want to have an significant disruption
> in traffic when we are adding/removing the VF. It seems like we would
>need to invalidate any entries that were configured for the virtio_net
>and reestablish them on the new team interface. Part of the criteria
>we have been working with is that we should be able to transition from
>having a VF to not or vice versa without seeing any significant
>disruption in the traffic.
What? You have routes on the team netdev. virtio_net and VF are only
slaves. What are you talking about? I don't get it :/
>
>Also how does this handle any static configuration? I am assuming that
>everything here assumes the team will be brought up as soon as it is
>seen and assigned a DHCP address.
Again. You configure whatever you need on the team netdev.
>
>The solution as you have proposed seems problematic at best. I don't
>see how the team solution works without introducing some sort of
>traffic disruption to either add/remove the VF and bring up/tear down
>the team interface. At that point we might as well just give up on
>this piece of live migration support entirely since the disruption was
>what we were trying to avoid. We might as well just hotplug out the VF
>and hotplug in a virtio at the same bus device and function number and
>just let udev take care of renaming it for us. The idea was supposed
>to be a seamless transition between the two interfaces.
Alex. What you are trying to do in this patchset and what netvsc does it
essentialy in-driver bonding. Same thing mechanism, rx_handler,
everything. I don't really understand what are you talking about. With
use of team you will get exactly the same behaviour.
^ permalink raw reply
* Re: [RFC PATCH v3 0/3] Enable virtio_net to act as a backup for a passthru device
From: Alexander Duyck @ 2018-02-21 16:49 UTC (permalink / raw)
To: Jiri Pirko
Cc: Duyck, Alexander H, virtio-dev, Michael S. Tsirkin,
Jakub Kicinski, Samudrala, Sridhar, virtualization, Siwei Liu,
Netdev, David Miller
In-Reply-To: <20180221161105.GC1996@nanopsycho>
On Wed, Feb 21, 2018 at 8:11 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Wed, Feb 21, 2018 at 04:56:48PM CET, alexander.duyck@gmail.com wrote:
>>On Wed, Feb 21, 2018 at 1:51 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> Tue, Feb 20, 2018 at 11:33:56PM CET, kubakici@wp.pl wrote:
>>>>On Tue, 20 Feb 2018 21:14:10 +0100, Jiri Pirko wrote:
>>>>> Yeah, I can see it now :( I guess that the ship has sailed and we are
>>>>> stuck with this ugly thing forever...
>>>>>
>>>>> Could you at least make some common code that is shared in between
>>>>> netvsc and virtio_net so this is handled in exacly the same way in both?
>>>>
>>>>IMHO netvsc is a vendor specific driver which made a mistake on what
>>>>behaviour it provides (or tried to align itself with Windows SR-IOV).
>>>>Let's not make a far, far more commonly deployed and important driver
>>>>(virtio) bug-compatible with netvsc.
>>>
>>> Yeah. netvsc solution is a dangerous precedent here and in my opinition
>>> it was a huge mistake to merge it. I personally would vote to unmerge it
>>> and make the solution based on team/bond.
>>>
>>>
>>>>
>>>>To Jiri's initial comments, I feel the same way, in fact I've talked to
>>>>the NetworkManager guys to get auto-bonding based on MACs handled in
>>>>user space. I think it may very well get done in next versions of NM,
>>>>but isn't done yet. Stephen also raised the point that not everybody is
>>>>using NM.
>>>
>>> Can be done in NM, networkd or other network management tools.
>>> Even easier to do this in teamd and let them all benefit.
>>>
>>> Actually, I took a stab to implement this in teamd. Took me like an hour
>>> and half.
>>>
>>> You can just run teamd with config option "kidnap" like this:
>>> # teamd/teamd -c '{"kidnap": true }'
>>>
>>> Whenever teamd sees another netdev to appear with the same mac as his,
>>> or whenever teamd sees another netdev to change mac to his,
>>> it enslaves it.
>>>
>>> Here's the patch (quick and dirty):
>>>
>>> Subject: [patch teamd] teamd: introduce kidnap feature
>>>
>>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>>
>>So this doesn't really address the original problem we were trying to
>>solve. You asked earlier why the netdev name mattered and it mostly
>>has to do with configuration. Specifically what our patch is
>>attempting to resolve is the issue of how to allow a cloud provider to
>>upgrade their customer to SR-IOV support and live migration without
>>requiring them to reconfigure their guest. So the general idea with
>>our patch is to take a VM that is running with virtio_net only and
>>allow it to instead spawn a virtio_bypass master using the same netdev
>>name as the original virtio, and then have the virtio_net and VF come
>>up and be enslaved by the bypass interface. Doing it this way we can
>>allow for multi-vendor SR-IOV live migration support using a guest
>>that was originally configured for virtio only.
>>
>>The problem with your solution is we already have teaming and bonding
>>as you said. There is already a write-up from Red Hat on how to do it
>>(https://access.redhat.com/documentation/en-us/red_hat_virtualization/4.1/html/virtual_machine_management_guide/sect-migrating_virtual_machines_between_hosts).
>>That is all well and good as long as you are willing to keep around
>>two VM images, one for virtio, and one for SR-IOV with live migration.
>
> You don't need 2 images. You need only one. The one with the team setup.
> That's it. If another netdev with the same mac appears, teamd will
> enslave it and run traffic on it. If not, ok, you'll go only through
> virtio_net.
Isn't that going to cause the routing table to get messed up when we
rearrange the netdevs? We don't want to have an significant disruption
in traffic when we are adding/removing the VF. It seems like we would
need to invalidate any entries that were configured for the virtio_net
and reestablish them on the new team interface. Part of the criteria
we have been working with is that we should be able to transition from
having a VF to not or vice versa without seeing any significant
disruption in the traffic.
Also how does this handle any static configuration? I am assuming that
everything here assumes the team will be brought up as soon as it is
seen and assigned a DHCP address.
The solution as you have proposed seems problematic at best. I don't
see how the team solution works without introducing some sort of
traffic disruption to either add/remove the VF and bring up/tear down
the team interface. At that point we might as well just give up on
this piece of live migration support entirely since the disruption was
what we were trying to avoid. We might as well just hotplug out the VF
and hotplug in a virtio at the same bus device and function number and
just let udev take care of renaming it for us. The idea was supposed
to be a seamless transition between the two interfaces.
^ permalink raw reply
* Re: [RFC PATCH v3 0/3] Enable virtio_net to act as a backup for a passthru device
From: Jiri Pirko @ 2018-02-21 16:11 UTC (permalink / raw)
To: Alexander Duyck
Cc: Duyck, Alexander H, virtio-dev, Michael S. Tsirkin,
Jakub Kicinski, Samudrala, Sridhar, virtualization, Siwei Liu,
Netdev, David Miller
In-Reply-To: <CAKgT0UdyhrVr=pYZb=AJq9sWWUVb_BadbJTcqY1AwHHTw8cmQw@mail.gmail.com>
Wed, Feb 21, 2018 at 04:56:48PM CET, alexander.duyck@gmail.com wrote:
>On Wed, Feb 21, 2018 at 1:51 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Tue, Feb 20, 2018 at 11:33:56PM CET, kubakici@wp.pl wrote:
>>>On Tue, 20 Feb 2018 21:14:10 +0100, Jiri Pirko wrote:
>>>> Yeah, I can see it now :( I guess that the ship has sailed and we are
>>>> stuck with this ugly thing forever...
>>>>
>>>> Could you at least make some common code that is shared in between
>>>> netvsc and virtio_net so this is handled in exacly the same way in both?
>>>
>>>IMHO netvsc is a vendor specific driver which made a mistake on what
>>>behaviour it provides (or tried to align itself with Windows SR-IOV).
>>>Let's not make a far, far more commonly deployed and important driver
>>>(virtio) bug-compatible with netvsc.
>>
>> Yeah. netvsc solution is a dangerous precedent here and in my opinition
>> it was a huge mistake to merge it. I personally would vote to unmerge it
>> and make the solution based on team/bond.
>>
>>
>>>
>>>To Jiri's initial comments, I feel the same way, in fact I've talked to
>>>the NetworkManager guys to get auto-bonding based on MACs handled in
>>>user space. I think it may very well get done in next versions of NM,
>>>but isn't done yet. Stephen also raised the point that not everybody is
>>>using NM.
>>
>> Can be done in NM, networkd or other network management tools.
>> Even easier to do this in teamd and let them all benefit.
>>
>> Actually, I took a stab to implement this in teamd. Took me like an hour
>> and half.
>>
>> You can just run teamd with config option "kidnap" like this:
>> # teamd/teamd -c '{"kidnap": true }'
>>
>> Whenever teamd sees another netdev to appear with the same mac as his,
>> or whenever teamd sees another netdev to change mac to his,
>> it enslaves it.
>>
>> Here's the patch (quick and dirty):
>>
>> Subject: [patch teamd] teamd: introduce kidnap feature
>>
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>
>So this doesn't really address the original problem we were trying to
>solve. You asked earlier why the netdev name mattered and it mostly
>has to do with configuration. Specifically what our patch is
>attempting to resolve is the issue of how to allow a cloud provider to
>upgrade their customer to SR-IOV support and live migration without
>requiring them to reconfigure their guest. So the general idea with
>our patch is to take a VM that is running with virtio_net only and
>allow it to instead spawn a virtio_bypass master using the same netdev
>name as the original virtio, and then have the virtio_net and VF come
>up and be enslaved by the bypass interface. Doing it this way we can
>allow for multi-vendor SR-IOV live migration support using a guest
>that was originally configured for virtio only.
>
>The problem with your solution is we already have teaming and bonding
>as you said. There is already a write-up from Red Hat on how to do it
>(https://access.redhat.com/documentation/en-us/red_hat_virtualization/4.1/html/virtual_machine_management_guide/sect-migrating_virtual_machines_between_hosts).
>That is all well and good as long as you are willing to keep around
>two VM images, one for virtio, and one for SR-IOV with live migration.
You don't need 2 images. You need only one. The one with the team setup.
That's it. If another netdev with the same mac appears, teamd will
enslave it and run traffic on it. If not, ok, you'll go only through
virtio_net.
>The problem is nobody wants to do that. What they want is to maintain
>one guest image and if they decide to upgrade to SR-IOV they still
>want their live migration and they don't want to have to reconfigure
>the guest.
>
>That said it does seem to make the existing Red Hat solution easier to
>manage since you wouldn't be guessing at ifname so I have provided
>some feedback below.
>
>> ---
>> include/team.h | 7 +++++++
>> libteam/ifinfo.c | 20 ++++++++++++++++++++
>> teamd/teamd.c | 17 +++++++++++++++++
>> teamd/teamd.h | 5 +++++
>> teamd/teamd_events.c | 17 +++++++++++++++++
>> teamd/teamd_ifinfo_watch.c | 9 +++++++++
>> teamd/teamd_per_port.c | 7 ++++++-
>> 7 files changed, 81 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/team.h b/include/team.h
>> index 9ae517d..b0c19c8 100644
>> --- a/include/team.h
>> +++ b/include/team.h
>> @@ -137,6 +137,13 @@ struct team_ifinfo *team_get_next_ifinfo(struct team_handle *th,
>> #define team_for_each_ifinfo(ifinfo, th) \
>> for (ifinfo = team_get_next_ifinfo(th, NULL); ifinfo; \
>> ifinfo = team_get_next_ifinfo(th, ifinfo))
>> +
>> +struct team_ifinfo *team_get_next_unlinked_ifinfo(struct team_handle *th,
>> + struct team_ifinfo *ifinfo);
>> +#define team_for_each_unlinked_ifinfo(ifinfo, th) \
>> + for (ifinfo = team_get_next_unlinked_ifinfo(th, NULL); ifinfo; \
>> + ifinfo = team_get_next_unlinked_ifinfo(th, ifinfo))
>> +
>> /* ifinfo getters */
>> bool team_is_ifinfo_removed(struct team_ifinfo *ifinfo);
>> uint32_t team_get_ifinfo_ifindex(struct team_ifinfo *ifinfo);
>> diff --git a/libteam/ifinfo.c b/libteam/ifinfo.c
>> index 5c32a9c..8f9548e 100644
>> --- a/libteam/ifinfo.c
>> +++ b/libteam/ifinfo.c
>> @@ -494,6 +494,26 @@ struct team_ifinfo *team_get_next_ifinfo(struct team_handle *th,
>> return NULL;
>> }
>>
>> +/**
>> + * @param th libteam library context
>> + * @param ifinfo ifinfo structure
>> + *
>> + * @details Get next unlinked ifinfo in list.
>> + *
>> + * @return Ifinfo next to ifinfo passed.
>> + **/
>> +TEAM_EXPORT
>> +struct team_ifinfo *team_get_next_unlinked_ifinfo(struct team_handle *th,
>> + struct team_ifinfo *ifinfo)
>> +{
>> + do {
>> + ifinfo = list_get_next_node_entry(&th->ifinfo_list, ifinfo, list);
>> + if (ifinfo && !ifinfo->linked)
>> + return ifinfo;
>> + } while (ifinfo);
>> + return NULL;
>> +}
>> +
>> /**
>> * @param ifinfo ifinfo structure
>> *
>> diff --git a/teamd/teamd.c b/teamd/teamd.c
>> index aac2511..069c7f0 100644
>> --- a/teamd/teamd.c
>> +++ b/teamd/teamd.c
>> @@ -926,8 +926,25 @@ static int teamd_event_watch_port_added(struct teamd_context *ctx,
>> return 0;
>> }
>>
>> +static int teamd_event_watch_unlinked_hwaddr_changed(struct teamd_context *ctx,
>> + struct team_ifinfo *ifinfo,
>> + void *priv)
>> +{
>> + int err;
>> + bool kidnap;
>> +
>> + err = teamd_config_bool_get(ctx, &kidnap, "$.kidnap");
>> + if (err || !kidnap ||
>> + ctx->hwaddr_len != team_get_ifinfo_hwaddr_len(ifinfo) ||
>> + memcmp(team_get_ifinfo_hwaddr(ifinfo),
>> + ctx->hwaddr, ctx->hwaddr_len))
>> + return 0;
>> + return teamd_port_add(ctx, team_get_ifinfo_ifindex(ifinfo));
>> +}
>> +
>
>So I am not sure about the name of this function. It seems to imply
>that we want to capture a device if it changed its MAC address to
>match the one we are using. I suppose that works if we are making this
>a genreric thing that can run on any netdev, but our focus is virtio
>and VFs. In the grand scheme of things they shouldn't be able to
>change their MAC address in most environments that we will care about.
>That was one of the reasons why we didn't bother supporting a MAC
>change in our code since the hypervisor should have this locked and
>attempting to use a different MAC address would likely trigger the VM
>as being flagged as malicious.
This cb is called whenever mac changes, but also when netdev appears
(hwaddr changes from 00:00:00:00:00:00 and hwaddr_len from 0)
>
>> static const struct teamd_event_watch_ops teamd_port_watch_ops = {
>> .port_added = teamd_event_watch_port_added,
>> + .unlinked_hwaddr_changed = teamd_event_watch_unlinked_hwaddr_changed,
>> };
>>
>> static int teamd_port_watch_init(struct teamd_context *ctx)
>> diff --git a/teamd/teamd.h b/teamd/teamd.h
>> index 5dbfb9b..171a8d1 100644
>> --- a/teamd/teamd.h
>> +++ b/teamd/teamd.h
>> @@ -189,6 +189,8 @@ struct teamd_event_watch_ops {
>> struct teamd_port *tdport, void *priv);
>> int (*port_ifname_changed)(struct teamd_context *ctx,
>> struct teamd_port *tdport, void *priv);
>> + int (*unlinked_hwaddr_changed)(struct teamd_context *ctx,
>> + struct team_ifinfo *ifinfo, void *priv);
>> int (*option_changed)(struct teamd_context *ctx,
>> struct team_option *option, void *priv);
>> char *option_changed_match_name;
>> @@ -210,6 +212,8 @@ int teamd_event_ifinfo_ifname_changed(struct teamd_context *ctx,
>> struct team_ifinfo *ifinfo);
>> int teamd_event_ifinfo_admin_state_changed(struct teamd_context *ctx,
>> struct team_ifinfo *ifinfo);
>> +int teamd_event_unlinked_ifinfo_hwaddr_changed(struct teamd_context *ctx,
>> + struct team_ifinfo *ifinfo);
>> int teamd_events_init(struct teamd_context *ctx);
>> void teamd_events_fini(struct teamd_context *ctx);
>> int teamd_event_watch_register(struct teamd_context *ctx,
>> @@ -313,6 +317,7 @@ static inline unsigned int teamd_port_count(struct teamd_context *ctx)
>> return ctx->port_obj_list_count;
>> }
>>
>> +int teamd_port_add(struct teamd_context *ctx, uint32_t ifindex);
>> int teamd_port_add_ifname(struct teamd_context *ctx, const char *port_name);
>> int teamd_port_remove_ifname(struct teamd_context *ctx, const char *port_name);
>> int teamd_port_remove_all(struct teamd_context *ctx);
>> diff --git a/teamd/teamd_events.c b/teamd/teamd_events.c
>> index 1a95974..a377090 100644
>> --- a/teamd/teamd_events.c
>> +++ b/teamd/teamd_events.c
>> @@ -184,6 +184,23 @@ int teamd_event_ifinfo_admin_state_changed(struct teamd_context *ctx,
>> return 0;
>> }
>>
>> +int teamd_event_unlinked_ifinfo_hwaddr_changed(struct teamd_context *ctx,
>> + struct team_ifinfo *ifinfo)
>> +{
>> + struct event_watch_item *watch;
>> + int err;
>> +
>> + list_for_each_node_entry(watch, &ctx->event_watch_list, list) {
>> + if (watch->ops->unlinked_hwaddr_changed) {
>
>I would probably flip the order of this. There is no point in doing
>the loop if unlinked_hwaddr_changed is NULL. So you could probably
>check for the function pointer first and then run the loop if it is
>set.
Sure. As I said, quick and dirty patch :)
>
>> + err = watch->ops->unlinked_hwaddr_changed(ctx, ifinfo,
>> + watch->priv);
>> + if (err)
>> + return err;
>> + }
>> + }
>> + return 0;
>> +}
>> +
>> int teamd_events_init(struct teamd_context *ctx)
>> {
>> list_init(&ctx->event_watch_list);
>> diff --git a/teamd/teamd_ifinfo_watch.c b/teamd/teamd_ifinfo_watch.c
>> index f334ff6..8d01a76 100644
>> --- a/teamd/teamd_ifinfo_watch.c
>> +++ b/teamd/teamd_ifinfo_watch.c
>> @@ -60,6 +60,15 @@ static int ifinfo_change_handler_func(struct team_handle *th, void *priv,
>> return err;
>> }
>> }
>> +
>> + team_for_each_unlinked_ifinfo(ifinfo, th) {
>> + if (team_is_ifinfo_hwaddr_changed(ifinfo) ||
>> + team_is_ifinfo_hwaddr_len_changed(ifinfo)) {
>> + err = teamd_event_unlinked_ifinfo_hwaddr_changed(ctx, ifinfo);
>> + if (err)
>> + return err;
>> + }
>> + }
>
>I guess this is needed for the generic case, but as I said we wouldn't
>probably need to worry about this in the VF/virtio case as the VM is
>probably locked to a specific MAC address.
>
>Also I am not sure about this bit. It seems like this is only checking
>for the HW addr being changed. Is that bit set if a new interface is
>registered? I haven't worked on teamd so I am not familiar with how it
>handles new interfaces. Also how does this handle existing interfaces
>that were registered before you started this?
See my reply above.
>
>> return 0;
>> }
>>
>> diff --git a/teamd/teamd_per_port.c b/teamd/teamd_per_port.c
>> index 09d1dc7..21e1bda 100644
>> --- a/teamd/teamd_per_port.c
>> +++ b/teamd/teamd_per_port.c
>> @@ -331,6 +331,11 @@ next_one:
>> return tdport;
>> }
>>
>> +int teamd_port_add(struct teamd_context *ctx, uint32_t ifindex)
>> +{
>> + return team_port_add(ctx->th, ifindex);
>> +}
>> +
>> int teamd_port_add_ifname(struct teamd_context *ctx, const char *port_name)
>> {
>> uint32_t ifindex;
>> @@ -338,7 +343,7 @@ int teamd_port_add_ifname(struct teamd_context *ctx, const char *port_name)
>> ifindex = team_ifname2ifindex(ctx->th, port_name);
>> teamd_log_dbg("%s: Adding port (found ifindex \"%d\").",
>> port_name, ifindex);
>> - return team_port_add(ctx->th, ifindex);
>> + return teamd_port_add(ctx, ifindex);
>> }
>>
>> static int teamd_port_remove(struct teamd_context *ctx,
^ permalink raw reply
* Re: [RFC PATCH v3 0/3] Enable virtio_net to act as a backup for a passthru device
From: Alexander Duyck @ 2018-02-21 15:56 UTC (permalink / raw)
To: Jiri Pirko
Cc: Duyck, Alexander H, virtio-dev, Michael S. Tsirkin,
Jakub Kicinski, Samudrala, Sridhar, virtualization, Siwei Liu,
Netdev, David Miller
In-Reply-To: <20180221095159.GA1996@nanopsycho>
On Wed, Feb 21, 2018 at 1:51 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Tue, Feb 20, 2018 at 11:33:56PM CET, kubakici@wp.pl wrote:
>>On Tue, 20 Feb 2018 21:14:10 +0100, Jiri Pirko wrote:
>>> Yeah, I can see it now :( I guess that the ship has sailed and we are
>>> stuck with this ugly thing forever...
>>>
>>> Could you at least make some common code that is shared in between
>>> netvsc and virtio_net so this is handled in exacly the same way in both?
>>
>>IMHO netvsc is a vendor specific driver which made a mistake on what
>>behaviour it provides (or tried to align itself with Windows SR-IOV).
>>Let's not make a far, far more commonly deployed and important driver
>>(virtio) bug-compatible with netvsc.
>
> Yeah. netvsc solution is a dangerous precedent here and in my opinition
> it was a huge mistake to merge it. I personally would vote to unmerge it
> and make the solution based on team/bond.
>
>
>>
>>To Jiri's initial comments, I feel the same way, in fact I've talked to
>>the NetworkManager guys to get auto-bonding based on MACs handled in
>>user space. I think it may very well get done in next versions of NM,
>>but isn't done yet. Stephen also raised the point that not everybody is
>>using NM.
>
> Can be done in NM, networkd or other network management tools.
> Even easier to do this in teamd and let them all benefit.
>
> Actually, I took a stab to implement this in teamd. Took me like an hour
> and half.
>
> You can just run teamd with config option "kidnap" like this:
> # teamd/teamd -c '{"kidnap": true }'
>
> Whenever teamd sees another netdev to appear with the same mac as his,
> or whenever teamd sees another netdev to change mac to his,
> it enslaves it.
>
> Here's the patch (quick and dirty):
>
> Subject: [patch teamd] teamd: introduce kidnap feature
>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
So this doesn't really address the original problem we were trying to
solve. You asked earlier why the netdev name mattered and it mostly
has to do with configuration. Specifically what our patch is
attempting to resolve is the issue of how to allow a cloud provider to
upgrade their customer to SR-IOV support and live migration without
requiring them to reconfigure their guest. So the general idea with
our patch is to take a VM that is running with virtio_net only and
allow it to instead spawn a virtio_bypass master using the same netdev
name as the original virtio, and then have the virtio_net and VF come
up and be enslaved by the bypass interface. Doing it this way we can
allow for multi-vendor SR-IOV live migration support using a guest
that was originally configured for virtio only.
The problem with your solution is we already have teaming and bonding
as you said. There is already a write-up from Red Hat on how to do it
(https://access.redhat.com/documentation/en-us/red_hat_virtualization/4.1/html/virtual_machine_management_guide/sect-migrating_virtual_machines_between_hosts).
That is all well and good as long as you are willing to keep around
two VM images, one for virtio, and one for SR-IOV with live migration.
The problem is nobody wants to do that. What they want is to maintain
one guest image and if they decide to upgrade to SR-IOV they still
want their live migration and they don't want to have to reconfigure
the guest.
That said it does seem to make the existing Red Hat solution easier to
manage since you wouldn't be guessing at ifname so I have provided
some feedback below.
> ---
> include/team.h | 7 +++++++
> libteam/ifinfo.c | 20 ++++++++++++++++++++
> teamd/teamd.c | 17 +++++++++++++++++
> teamd/teamd.h | 5 +++++
> teamd/teamd_events.c | 17 +++++++++++++++++
> teamd/teamd_ifinfo_watch.c | 9 +++++++++
> teamd/teamd_per_port.c | 7 ++++++-
> 7 files changed, 81 insertions(+), 1 deletion(-)
>
> diff --git a/include/team.h b/include/team.h
> index 9ae517d..b0c19c8 100644
> --- a/include/team.h
> +++ b/include/team.h
> @@ -137,6 +137,13 @@ struct team_ifinfo *team_get_next_ifinfo(struct team_handle *th,
> #define team_for_each_ifinfo(ifinfo, th) \
> for (ifinfo = team_get_next_ifinfo(th, NULL); ifinfo; \
> ifinfo = team_get_next_ifinfo(th, ifinfo))
> +
> +struct team_ifinfo *team_get_next_unlinked_ifinfo(struct team_handle *th,
> + struct team_ifinfo *ifinfo);
> +#define team_for_each_unlinked_ifinfo(ifinfo, th) \
> + for (ifinfo = team_get_next_unlinked_ifinfo(th, NULL); ifinfo; \
> + ifinfo = team_get_next_unlinked_ifinfo(th, ifinfo))
> +
> /* ifinfo getters */
> bool team_is_ifinfo_removed(struct team_ifinfo *ifinfo);
> uint32_t team_get_ifinfo_ifindex(struct team_ifinfo *ifinfo);
> diff --git a/libteam/ifinfo.c b/libteam/ifinfo.c
> index 5c32a9c..8f9548e 100644
> --- a/libteam/ifinfo.c
> +++ b/libteam/ifinfo.c
> @@ -494,6 +494,26 @@ struct team_ifinfo *team_get_next_ifinfo(struct team_handle *th,
> return NULL;
> }
>
> +/**
> + * @param th libteam library context
> + * @param ifinfo ifinfo structure
> + *
> + * @details Get next unlinked ifinfo in list.
> + *
> + * @return Ifinfo next to ifinfo passed.
> + **/
> +TEAM_EXPORT
> +struct team_ifinfo *team_get_next_unlinked_ifinfo(struct team_handle *th,
> + struct team_ifinfo *ifinfo)
> +{
> + do {
> + ifinfo = list_get_next_node_entry(&th->ifinfo_list, ifinfo, list);
> + if (ifinfo && !ifinfo->linked)
> + return ifinfo;
> + } while (ifinfo);
> + return NULL;
> +}
> +
> /**
> * @param ifinfo ifinfo structure
> *
> diff --git a/teamd/teamd.c b/teamd/teamd.c
> index aac2511..069c7f0 100644
> --- a/teamd/teamd.c
> +++ b/teamd/teamd.c
> @@ -926,8 +926,25 @@ static int teamd_event_watch_port_added(struct teamd_context *ctx,
> return 0;
> }
>
> +static int teamd_event_watch_unlinked_hwaddr_changed(struct teamd_context *ctx,
> + struct team_ifinfo *ifinfo,
> + void *priv)
> +{
> + int err;
> + bool kidnap;
> +
> + err = teamd_config_bool_get(ctx, &kidnap, "$.kidnap");
> + if (err || !kidnap ||
> + ctx->hwaddr_len != team_get_ifinfo_hwaddr_len(ifinfo) ||
> + memcmp(team_get_ifinfo_hwaddr(ifinfo),
> + ctx->hwaddr, ctx->hwaddr_len))
> + return 0;
> + return teamd_port_add(ctx, team_get_ifinfo_ifindex(ifinfo));
> +}
> +
So I am not sure about the name of this function. It seems to imply
that we want to capture a device if it changed its MAC address to
match the one we are using. I suppose that works if we are making this
a genreric thing that can run on any netdev, but our focus is virtio
and VFs. In the grand scheme of things they shouldn't be able to
change their MAC address in most environments that we will care about.
That was one of the reasons why we didn't bother supporting a MAC
change in our code since the hypervisor should have this locked and
attempting to use a different MAC address would likely trigger the VM
as being flagged as malicious.
> static const struct teamd_event_watch_ops teamd_port_watch_ops = {
> .port_added = teamd_event_watch_port_added,
> + .unlinked_hwaddr_changed = teamd_event_watch_unlinked_hwaddr_changed,
> };
>
> static int teamd_port_watch_init(struct teamd_context *ctx)
> diff --git a/teamd/teamd.h b/teamd/teamd.h
> index 5dbfb9b..171a8d1 100644
> --- a/teamd/teamd.h
> +++ b/teamd/teamd.h
> @@ -189,6 +189,8 @@ struct teamd_event_watch_ops {
> struct teamd_port *tdport, void *priv);
> int (*port_ifname_changed)(struct teamd_context *ctx,
> struct teamd_port *tdport, void *priv);
> + int (*unlinked_hwaddr_changed)(struct teamd_context *ctx,
> + struct team_ifinfo *ifinfo, void *priv);
> int (*option_changed)(struct teamd_context *ctx,
> struct team_option *option, void *priv);
> char *option_changed_match_name;
> @@ -210,6 +212,8 @@ int teamd_event_ifinfo_ifname_changed(struct teamd_context *ctx,
> struct team_ifinfo *ifinfo);
> int teamd_event_ifinfo_admin_state_changed(struct teamd_context *ctx,
> struct team_ifinfo *ifinfo);
> +int teamd_event_unlinked_ifinfo_hwaddr_changed(struct teamd_context *ctx,
> + struct team_ifinfo *ifinfo);
> int teamd_events_init(struct teamd_context *ctx);
> void teamd_events_fini(struct teamd_context *ctx);
> int teamd_event_watch_register(struct teamd_context *ctx,
> @@ -313,6 +317,7 @@ static inline unsigned int teamd_port_count(struct teamd_context *ctx)
> return ctx->port_obj_list_count;
> }
>
> +int teamd_port_add(struct teamd_context *ctx, uint32_t ifindex);
> int teamd_port_add_ifname(struct teamd_context *ctx, const char *port_name);
> int teamd_port_remove_ifname(struct teamd_context *ctx, const char *port_name);
> int teamd_port_remove_all(struct teamd_context *ctx);
> diff --git a/teamd/teamd_events.c b/teamd/teamd_events.c
> index 1a95974..a377090 100644
> --- a/teamd/teamd_events.c
> +++ b/teamd/teamd_events.c
> @@ -184,6 +184,23 @@ int teamd_event_ifinfo_admin_state_changed(struct teamd_context *ctx,
> return 0;
> }
>
> +int teamd_event_unlinked_ifinfo_hwaddr_changed(struct teamd_context *ctx,
> + struct team_ifinfo *ifinfo)
> +{
> + struct event_watch_item *watch;
> + int err;
> +
> + list_for_each_node_entry(watch, &ctx->event_watch_list, list) {
> + if (watch->ops->unlinked_hwaddr_changed) {
I would probably flip the order of this. There is no point in doing
the loop if unlinked_hwaddr_changed is NULL. So you could probably
check for the function pointer first and then run the loop if it is
set.
> + err = watch->ops->unlinked_hwaddr_changed(ctx, ifinfo,
> + watch->priv);
> + if (err)
> + return err;
> + }
> + }
> + return 0;
> +}
> +
> int teamd_events_init(struct teamd_context *ctx)
> {
> list_init(&ctx->event_watch_list);
> diff --git a/teamd/teamd_ifinfo_watch.c b/teamd/teamd_ifinfo_watch.c
> index f334ff6..8d01a76 100644
> --- a/teamd/teamd_ifinfo_watch.c
> +++ b/teamd/teamd_ifinfo_watch.c
> @@ -60,6 +60,15 @@ static int ifinfo_change_handler_func(struct team_handle *th, void *priv,
> return err;
> }
> }
> +
> + team_for_each_unlinked_ifinfo(ifinfo, th) {
> + if (team_is_ifinfo_hwaddr_changed(ifinfo) ||
> + team_is_ifinfo_hwaddr_len_changed(ifinfo)) {
> + err = teamd_event_unlinked_ifinfo_hwaddr_changed(ctx, ifinfo);
> + if (err)
> + return err;
> + }
> + }
I guess this is needed for the generic case, but as I said we wouldn't
probably need to worry about this in the VF/virtio case as the VM is
probably locked to a specific MAC address.
Also I am not sure about this bit. It seems like this is only checking
for the HW addr being changed. Is that bit set if a new interface is
registered? I haven't worked on teamd so I am not familiar with how it
handles new interfaces. Also how does this handle existing interfaces
that were registered before you started this?
> return 0;
> }
>
> diff --git a/teamd/teamd_per_port.c b/teamd/teamd_per_port.c
> index 09d1dc7..21e1bda 100644
> --- a/teamd/teamd_per_port.c
> +++ b/teamd/teamd_per_port.c
> @@ -331,6 +331,11 @@ next_one:
> return tdport;
> }
>
> +int teamd_port_add(struct teamd_context *ctx, uint32_t ifindex)
> +{
> + return team_port_add(ctx->th, ifindex);
> +}
> +
> int teamd_port_add_ifname(struct teamd_context *ctx, const char *port_name)
> {
> uint32_t ifindex;
> @@ -338,7 +343,7 @@ int teamd_port_add_ifname(struct teamd_context *ctx, const char *port_name)
> ifindex = team_ifname2ifindex(ctx->th, port_name);
> teamd_log_dbg("%s: Adding port (found ifindex \"%d\").",
> port_name, ifindex);
> - return team_port_add(ctx->th, ifindex);
> + return teamd_port_add(ctx, ifindex);
> }
>
> static int teamd_port_remove(struct teamd_context *ctx,
^ permalink raw reply
* Re: [RFC PATCH v3 0/3] Enable virtio_net to act as a backup for a passthru device
From: Jiri Pirko @ 2018-02-21 9:51 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Duyck, Alexander H, virtio-dev, Michael S. Tsirkin,
Samudrala, Sridhar, Alexander Duyck, virtualization, Siwei Liu,
Netdev, David Miller
In-Reply-To: <20180220143356.3467084d@cakuba.netronome.com>
Tue, Feb 20, 2018 at 11:33:56PM CET, kubakici@wp.pl wrote:
>On Tue, 20 Feb 2018 21:14:10 +0100, Jiri Pirko wrote:
>> Yeah, I can see it now :( I guess that the ship has sailed and we are
>> stuck with this ugly thing forever...
>>
>> Could you at least make some common code that is shared in between
>> netvsc and virtio_net so this is handled in exacly the same way in both?
>
>IMHO netvsc is a vendor specific driver which made a mistake on what
>behaviour it provides (or tried to align itself with Windows SR-IOV).
>Let's not make a far, far more commonly deployed and important driver
>(virtio) bug-compatible with netvsc.
Yeah. netvsc solution is a dangerous precedent here and in my opinition
it was a huge mistake to merge it. I personally would vote to unmerge it
and make the solution based on team/bond.
>
>To Jiri's initial comments, I feel the same way, in fact I've talked to
>the NetworkManager guys to get auto-bonding based on MACs handled in
>user space. I think it may very well get done in next versions of NM,
>but isn't done yet. Stephen also raised the point that not everybody is
>using NM.
Can be done in NM, networkd or other network management tools.
Even easier to do this in teamd and let them all benefit.
Actually, I took a stab to implement this in teamd. Took me like an hour
and half.
You can just run teamd with config option "kidnap" like this:
# teamd/teamd -c '{"kidnap": true }'
Whenever teamd sees another netdev to appear with the same mac as his,
or whenever teamd sees another netdev to change mac to his,
it enslaves it.
Here's the patch (quick and dirty):
Subject: [patch teamd] teamd: introduce kidnap feature
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
include/team.h | 7 +++++++
libteam/ifinfo.c | 20 ++++++++++++++++++++
teamd/teamd.c | 17 +++++++++++++++++
teamd/teamd.h | 5 +++++
teamd/teamd_events.c | 17 +++++++++++++++++
teamd/teamd_ifinfo_watch.c | 9 +++++++++
teamd/teamd_per_port.c | 7 ++++++-
7 files changed, 81 insertions(+), 1 deletion(-)
diff --git a/include/team.h b/include/team.h
index 9ae517d..b0c19c8 100644
--- a/include/team.h
+++ b/include/team.h
@@ -137,6 +137,13 @@ struct team_ifinfo *team_get_next_ifinfo(struct team_handle *th,
#define team_for_each_ifinfo(ifinfo, th) \
for (ifinfo = team_get_next_ifinfo(th, NULL); ifinfo; \
ifinfo = team_get_next_ifinfo(th, ifinfo))
+
+struct team_ifinfo *team_get_next_unlinked_ifinfo(struct team_handle *th,
+ struct team_ifinfo *ifinfo);
+#define team_for_each_unlinked_ifinfo(ifinfo, th) \
+ for (ifinfo = team_get_next_unlinked_ifinfo(th, NULL); ifinfo; \
+ ifinfo = team_get_next_unlinked_ifinfo(th, ifinfo))
+
/* ifinfo getters */
bool team_is_ifinfo_removed(struct team_ifinfo *ifinfo);
uint32_t team_get_ifinfo_ifindex(struct team_ifinfo *ifinfo);
diff --git a/libteam/ifinfo.c b/libteam/ifinfo.c
index 5c32a9c..8f9548e 100644
--- a/libteam/ifinfo.c
+++ b/libteam/ifinfo.c
@@ -494,6 +494,26 @@ struct team_ifinfo *team_get_next_ifinfo(struct team_handle *th,
return NULL;
}
+/**
+ * @param th libteam library context
+ * @param ifinfo ifinfo structure
+ *
+ * @details Get next unlinked ifinfo in list.
+ *
+ * @return Ifinfo next to ifinfo passed.
+ **/
+TEAM_EXPORT
+struct team_ifinfo *team_get_next_unlinked_ifinfo(struct team_handle *th,
+ struct team_ifinfo *ifinfo)
+{
+ do {
+ ifinfo = list_get_next_node_entry(&th->ifinfo_list, ifinfo, list);
+ if (ifinfo && !ifinfo->linked)
+ return ifinfo;
+ } while (ifinfo);
+ return NULL;
+}
+
/**
* @param ifinfo ifinfo structure
*
diff --git a/teamd/teamd.c b/teamd/teamd.c
index aac2511..069c7f0 100644
--- a/teamd/teamd.c
+++ b/teamd/teamd.c
@@ -926,8 +926,25 @@ static int teamd_event_watch_port_added(struct teamd_context *ctx,
return 0;
}
+static int teamd_event_watch_unlinked_hwaddr_changed(struct teamd_context *ctx,
+ struct team_ifinfo *ifinfo,
+ void *priv)
+{
+ int err;
+ bool kidnap;
+
+ err = teamd_config_bool_get(ctx, &kidnap, "$.kidnap");
+ if (err || !kidnap ||
+ ctx->hwaddr_len != team_get_ifinfo_hwaddr_len(ifinfo) ||
+ memcmp(team_get_ifinfo_hwaddr(ifinfo),
+ ctx->hwaddr, ctx->hwaddr_len))
+ return 0;
+ return teamd_port_add(ctx, team_get_ifinfo_ifindex(ifinfo));
+}
+
static const struct teamd_event_watch_ops teamd_port_watch_ops = {
.port_added = teamd_event_watch_port_added,
+ .unlinked_hwaddr_changed = teamd_event_watch_unlinked_hwaddr_changed,
};
static int teamd_port_watch_init(struct teamd_context *ctx)
diff --git a/teamd/teamd.h b/teamd/teamd.h
index 5dbfb9b..171a8d1 100644
--- a/teamd/teamd.h
+++ b/teamd/teamd.h
@@ -189,6 +189,8 @@ struct teamd_event_watch_ops {
struct teamd_port *tdport, void *priv);
int (*port_ifname_changed)(struct teamd_context *ctx,
struct teamd_port *tdport, void *priv);
+ int (*unlinked_hwaddr_changed)(struct teamd_context *ctx,
+ struct team_ifinfo *ifinfo, void *priv);
int (*option_changed)(struct teamd_context *ctx,
struct team_option *option, void *priv);
char *option_changed_match_name;
@@ -210,6 +212,8 @@ int teamd_event_ifinfo_ifname_changed(struct teamd_context *ctx,
struct team_ifinfo *ifinfo);
int teamd_event_ifinfo_admin_state_changed(struct teamd_context *ctx,
struct team_ifinfo *ifinfo);
+int teamd_event_unlinked_ifinfo_hwaddr_changed(struct teamd_context *ctx,
+ struct team_ifinfo *ifinfo);
int teamd_events_init(struct teamd_context *ctx);
void teamd_events_fini(struct teamd_context *ctx);
int teamd_event_watch_register(struct teamd_context *ctx,
@@ -313,6 +317,7 @@ static inline unsigned int teamd_port_count(struct teamd_context *ctx)
return ctx->port_obj_list_count;
}
+int teamd_port_add(struct teamd_context *ctx, uint32_t ifindex);
int teamd_port_add_ifname(struct teamd_context *ctx, const char *port_name);
int teamd_port_remove_ifname(struct teamd_context *ctx, const char *port_name);
int teamd_port_remove_all(struct teamd_context *ctx);
diff --git a/teamd/teamd_events.c b/teamd/teamd_events.c
index 1a95974..a377090 100644
--- a/teamd/teamd_events.c
+++ b/teamd/teamd_events.c
@@ -184,6 +184,23 @@ int teamd_event_ifinfo_admin_state_changed(struct teamd_context *ctx,
return 0;
}
+int teamd_event_unlinked_ifinfo_hwaddr_changed(struct teamd_context *ctx,
+ struct team_ifinfo *ifinfo)
+{
+ struct event_watch_item *watch;
+ int err;
+
+ list_for_each_node_entry(watch, &ctx->event_watch_list, list) {
+ if (watch->ops->unlinked_hwaddr_changed) {
+ err = watch->ops->unlinked_hwaddr_changed(ctx, ifinfo,
+ watch->priv);
+ if (err)
+ return err;
+ }
+ }
+ return 0;
+}
+
int teamd_events_init(struct teamd_context *ctx)
{
list_init(&ctx->event_watch_list);
diff --git a/teamd/teamd_ifinfo_watch.c b/teamd/teamd_ifinfo_watch.c
index f334ff6..8d01a76 100644
--- a/teamd/teamd_ifinfo_watch.c
+++ b/teamd/teamd_ifinfo_watch.c
@@ -60,6 +60,15 @@ static int ifinfo_change_handler_func(struct team_handle *th, void *priv,
return err;
}
}
+
+ team_for_each_unlinked_ifinfo(ifinfo, th) {
+ if (team_is_ifinfo_hwaddr_changed(ifinfo) ||
+ team_is_ifinfo_hwaddr_len_changed(ifinfo)) {
+ err = teamd_event_unlinked_ifinfo_hwaddr_changed(ctx, ifinfo);
+ if (err)
+ return err;
+ }
+ }
return 0;
}
diff --git a/teamd/teamd_per_port.c b/teamd/teamd_per_port.c
index 09d1dc7..21e1bda 100644
--- a/teamd/teamd_per_port.c
+++ b/teamd/teamd_per_port.c
@@ -331,6 +331,11 @@ next_one:
return tdport;
}
+int teamd_port_add(struct teamd_context *ctx, uint32_t ifindex)
+{
+ return team_port_add(ctx->th, ifindex);
+}
+
int teamd_port_add_ifname(struct teamd_context *ctx, const char *port_name)
{
uint32_t ifindex;
@@ -338,7 +343,7 @@ int teamd_port_add_ifname(struct teamd_context *ctx, const char *port_name)
ifindex = team_ifname2ifindex(ctx->th, port_name);
teamd_log_dbg("%s: Adding port (found ifindex \"%d\").",
port_name, ifindex);
- return team_port_add(ctx->th, ifindex);
+ return teamd_port_add(ctx, ifindex);
}
static int teamd_port_remove(struct teamd_context *ctx,
--
2.14.3
^ permalink raw reply related
* Re: [RFC PATCH v3 0/3] Enable virtio_net to act as a backup for a passthru device
From: Jakub Kicinski @ 2018-02-20 22:33 UTC (permalink / raw)
To: Jiri Pirko, Samudrala, Sridhar, Alexander Duyck
Cc: Duyck, Alexander H, virtio-dev, Michael S. Tsirkin, Netdev,
virtualization, Siwei Liu, David Miller
In-Reply-To: <20180220201410.GF2031@nanopsycho>
On Tue, 20 Feb 2018 21:14:10 +0100, Jiri Pirko wrote:
> Yeah, I can see it now :( I guess that the ship has sailed and we are
> stuck with this ugly thing forever...
>
> Could you at least make some common code that is shared in between
> netvsc and virtio_net so this is handled in exacly the same way in both?
IMHO netvsc is a vendor specific driver which made a mistake on what
behaviour it provides (or tried to align itself with Windows SR-IOV).
Let's not make a far, far more commonly deployed and important driver
(virtio) bug-compatible with netvsc.
To Jiri's initial comments, I feel the same way, in fact I've talked to
the NetworkManager guys to get auto-bonding based on MACs handled in
user space. I think it may very well get done in next versions of NM,
but isn't done yet. Stephen also raised the point that not everybody is
using NM.
^ permalink raw reply
* Re: [RFC PATCH v3 0/3] Enable virtio_net to act as a backup for a passthru device
From: Alexander Duyck @ 2018-02-20 21:02 UTC (permalink / raw)
To: Jiri Pirko
Cc: Duyck, Alexander H, virtio-dev, Michael S. Tsirkin,
Jakub Kicinski, Samudrala, Sridhar, virtualization, Siwei Liu,
Netdev, David Miller
In-Reply-To: <20180220201410.GF2031@nanopsycho>
On Tue, Feb 20, 2018 at 12:14 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Tue, Feb 20, 2018 at 06:14:32PM CET, sridhar.samudrala@intel.com wrote:
>>On 2/20/2018 8:29 AM, Jiri Pirko wrote:
>>> Tue, Feb 20, 2018 at 05:04:29PM CET, alexander.duyck@gmail.com wrote:
>>> > On Tue, Feb 20, 2018 at 2:42 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> > > Fri, Feb 16, 2018 at 07:11:19PM CET, sridhar.samudrala@intel.com wrote:
>>> > > > Patch 1 introduces a new feature bit VIRTIO_NET_F_BACKUP that can be
>>> > > > used by hypervisor to indicate that virtio_net interface should act as
>>> > > > a backup for another device with the same MAC address.
>>> > > >
>>> > > > Ppatch 2 is in response to the community request for a 3 netdev
>>> > > > solution. However, it creates some issues we'll get into in a moment.
>>> > > > It extends virtio_net to use alternate datapath when available and
>>> > > > registered. When BACKUP feature is enabled, virtio_net driver creates
>>> > > > an additional 'bypass' netdev that acts as a master device and controls
>>> > > > 2 slave devices. The original virtio_net netdev is registered as
>>> > > > 'backup' netdev and a passthru/vf device with the same MAC gets
>>> > > > registered as 'active' netdev. Both 'bypass' and 'backup' netdevs are
>>> > > > associated with the same 'pci' device. The user accesses the network
>>> > > > interface via 'bypass' netdev. The 'bypass' netdev chooses 'active' netdev
>>> > > > as default for transmits when it is available with link up and running.
>>> > > Sorry, but this is ridiculous. You are apparently re-implemeting part
>>> > > of bonding driver as a part of NIC driver. Bond and team drivers
>>> > > are mature solutions, well tested, broadly used, with lots of issues
>>> > > resolved in the past. What you try to introduce is a weird shortcut
>>> > > that already has couple of issues as you mentioned and will certanly
>>> > > have many more. Also, I'm pretty sure that in future, someone comes up
>>> > > with ideas like multiple VFs, LACP and similar bonding things.
>>> > The problem with the bond and team drivers is they are too large and
>>> > have too many interfaces available for configuration so as a result
>>> > they can really screw this interface up.
>>> What? Too large is which sense? Why "too many interfaces" is a problem?
>>> Also, team has only one interface to userspace team-generic-netlink.
>>>
>>>
>>> > Essentially this is meant to be a bond that is more-or-less managed by
>>> > the host, not the guest. We want the host to be able to configure it
>>> How is it managed by the host? In your usecase the guest has 2 netdevs:
>>> virtio_net, pci vf.
>>> I don't see how host can do any managing of that, other than the
>>> obvious. But still, the active/backup decision is done in guest. This is
>>> a simple bond/team usecase. As I said, there is something needed to be
>>> implemented in userspace in order to handle re-appear of vf netdev.
>>> But that should be fairly easy to do in teamd.
>>
>>The host manages the active/backup decision by
>>- assigning the same MAC address to both VF and virtio interfaces
>>- setting a BACKUP feature bit on virtio that enables virtio to transparently
>>take
>> over the VFs datapath.
>>- only enable one datapath at anytime so that packets don't get looped back
>>- during live migration enable virtio datapth, unplug vf on the source and
>>replug
>> vf on the destination.
>>
>>The VM is not expected and doesn't have any control of setting the MAC
>>address
>>or bringing up/down the links.
>>
>>This is the model that is currently supported with netvsc driver on Azure.
>
> Yeah, I can see it now :( I guess that the ship has sailed and we are
> stuck with this ugly thing forever...
>
> Could you at least make some common code that is shared in between
> netvsc and virtio_net so this is handled in exacly the same way in both?
>
> The fact that the netvsc/virtio_net kidnaps a netdev only because it
> has the same mac is going to give me some serious nighmares...
> I think we need to introduce some more strict checks.
In order for that to work we need to settle on a model for these. The
issue is that netvsc is using what we refer to as the "2 netdev" model
where they don't expose the paravirtual interface as its own netdev.
The opinion of Jakub and others has been that we should do a "3
netdev" model in the case of virtio_net since otherwise we will lose
functionality such as in-driver XDP and have to deal with an extra set
of qdiscs and Tx queue locks on transmit path.
Really at this point I am good either way, but we need to probably
have Stephen, Jakub, and whoever else had an opinion on the matter
sort out the 2 vs 3 argument before we could proceed on that. Most of
patch 2 in the set can easily be broken out into a separate file later
if we decide to go that route.
Thanks.
- Alex
^ permalink raw reply
* Re: [RFC PATCH v3 0/3] Enable virtio_net to act as a backup for a passthru device
From: Jiri Pirko @ 2018-02-20 20:14 UTC (permalink / raw)
To: Samudrala, Sridhar
Cc: Duyck, Alexander H, virtio-dev, Michael S. Tsirkin,
Jakub Kicinski, Netdev, Alexander Duyck, virtualization,
Siwei Liu, David Miller
In-Reply-To: <509bbbf9-4db7-dc78-a05e-403452a7407a@intel.com>
Tue, Feb 20, 2018 at 06:14:32PM CET, sridhar.samudrala@intel.com wrote:
>On 2/20/2018 8:29 AM, Jiri Pirko wrote:
>> Tue, Feb 20, 2018 at 05:04:29PM CET, alexander.duyck@gmail.com wrote:
>> > On Tue, Feb 20, 2018 at 2:42 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> > > Fri, Feb 16, 2018 at 07:11:19PM CET, sridhar.samudrala@intel.com wrote:
>> > > > Patch 1 introduces a new feature bit VIRTIO_NET_F_BACKUP that can be
>> > > > used by hypervisor to indicate that virtio_net interface should act as
>> > > > a backup for another device with the same MAC address.
>> > > >
>> > > > Ppatch 2 is in response to the community request for a 3 netdev
>> > > > solution. However, it creates some issues we'll get into in a moment.
>> > > > It extends virtio_net to use alternate datapath when available and
>> > > > registered. When BACKUP feature is enabled, virtio_net driver creates
>> > > > an additional 'bypass' netdev that acts as a master device and controls
>> > > > 2 slave devices. The original virtio_net netdev is registered as
>> > > > 'backup' netdev and a passthru/vf device with the same MAC gets
>> > > > registered as 'active' netdev. Both 'bypass' and 'backup' netdevs are
>> > > > associated with the same 'pci' device. The user accesses the network
>> > > > interface via 'bypass' netdev. The 'bypass' netdev chooses 'active' netdev
>> > > > as default for transmits when it is available with link up and running.
>> > > Sorry, but this is ridiculous. You are apparently re-implemeting part
>> > > of bonding driver as a part of NIC driver. Bond and team drivers
>> > > are mature solutions, well tested, broadly used, with lots of issues
>> > > resolved in the past. What you try to introduce is a weird shortcut
>> > > that already has couple of issues as you mentioned and will certanly
>> > > have many more. Also, I'm pretty sure that in future, someone comes up
>> > > with ideas like multiple VFs, LACP and similar bonding things.
>> > The problem with the bond and team drivers is they are too large and
>> > have too many interfaces available for configuration so as a result
>> > they can really screw this interface up.
>> What? Too large is which sense? Why "too many interfaces" is a problem?
>> Also, team has only one interface to userspace team-generic-netlink.
>>
>>
>> > Essentially this is meant to be a bond that is more-or-less managed by
>> > the host, not the guest. We want the host to be able to configure it
>> How is it managed by the host? In your usecase the guest has 2 netdevs:
>> virtio_net, pci vf.
>> I don't see how host can do any managing of that, other than the
>> obvious. But still, the active/backup decision is done in guest. This is
>> a simple bond/team usecase. As I said, there is something needed to be
>> implemented in userspace in order to handle re-appear of vf netdev.
>> But that should be fairly easy to do in teamd.
>
>The host manages the active/backup decision by
>- assigning the same MAC address to both VF and virtio interfaces
>- setting a BACKUP feature bit on virtio that enables virtio to transparently
>take
> over the VFs datapath.
>- only enable one datapath at anytime so that packets don't get looped back
>- during live migration enable virtio datapth, unplug vf on the source and
>replug
> vf on the destination.
>
>The VM is not expected and doesn't have any control of setting the MAC
>address
>or bringing up/down the links.
>
>This is the model that is currently supported with netvsc driver on Azure.
Yeah, I can see it now :( I guess that the ship has sailed and we are
stuck with this ugly thing forever...
Could you at least make some common code that is shared in between
netvsc and virtio_net so this is handled in exacly the same way in both?
The fact that the netvsc/virtio_net kidnaps a netdev only because it
has the same mac is going to give me some serious nighmares...
I think we need to introduce some more strict checks.
^ permalink raw reply
* Re: [RFC PATCH v3 0/3] Enable virtio_net to act as a backup for a passthru device
From: Jiri Pirko @ 2018-02-20 19:53 UTC (permalink / raw)
To: Alexander Duyck
Cc: Duyck, Alexander H, virtio-dev, Michael S. Tsirkin,
Jakub Kicinski, Sridhar Samudrala, virtualization, Siwei Liu,
Netdev, David Miller
In-Reply-To: <CAKgT0UehiC9O6JW9kjc+h9oHiHnCsqfExzawq+N51yV2tb=zwA@mail.gmail.com>
Tue, Feb 20, 2018 at 06:23:49PM CET, alexander.duyck@gmail.com wrote:
>On Tue, Feb 20, 2018 at 8:29 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Tue, Feb 20, 2018 at 05:04:29PM CET, alexander.duyck@gmail.com wrote:
>>>On Tue, Feb 20, 2018 at 2:42 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>> Fri, Feb 16, 2018 at 07:11:19PM CET, sridhar.samudrala@intel.com wrote:
>>>>>Patch 1 introduces a new feature bit VIRTIO_NET_F_BACKUP that can be
>>>>>used by hypervisor to indicate that virtio_net interface should act as
>>>>>a backup for another device with the same MAC address.
>>>>>
>>>>>Ppatch 2 is in response to the community request for a 3 netdev
>>>>>solution. However, it creates some issues we'll get into in a moment.
>>>>>It extends virtio_net to use alternate datapath when available and
>>>>>registered. When BACKUP feature is enabled, virtio_net driver creates
>>>>>an additional 'bypass' netdev that acts as a master device and controls
>>>>>2 slave devices. The original virtio_net netdev is registered as
>>>>>'backup' netdev and a passthru/vf device with the same MAC gets
>>>>>registered as 'active' netdev. Both 'bypass' and 'backup' netdevs are
>>>>>associated with the same 'pci' device. The user accesses the network
>>>>>interface via 'bypass' netdev. The 'bypass' netdev chooses 'active' netdev
>>>>>as default for transmits when it is available with link up and running.
>>>>
>>>> Sorry, but this is ridiculous. You are apparently re-implemeting part
>>>> of bonding driver as a part of NIC driver. Bond and team drivers
>>>> are mature solutions, well tested, broadly used, with lots of issues
>>>> resolved in the past. What you try to introduce is a weird shortcut
>>>> that already has couple of issues as you mentioned and will certanly
>>>> have many more. Also, I'm pretty sure that in future, someone comes up
>>>> with ideas like multiple VFs, LACP and similar bonding things.
>>>
>>>The problem with the bond and team drivers is they are too large and
>>>have too many interfaces available for configuration so as a result
>>>they can really screw this interface up.
>>
>> What? Too large is which sense? Why "too many interfaces" is a problem?
>> Also, team has only one interface to userspace team-generic-netlink.
>
>Specifically I was working with bond. I had overlooked team for the
>most part since it required an additional userspace daemon which
>basically broke our requirement of no user-space intervention.
Why? That sound artificial. Why the userspace cannot be part of the
solution?
>
>I was trying to focus on just doing an active/backup setup. The
>problem is there are debugfs, sysfs, and procfs interfaces exposed
>that we don't need and/or want. Adding any sort of interface to
>exclude these would just bloat up the bonding driver, and leaving them
>in would just be confusing since they would all need to be ignored. In
>addition the steps needed to get the name to come out the same as the
>original virtio interface would just bloat up bonding.
Why to you care about "name"? it's a netdev, isn't it all that matters?
The viewpoint of the user inside vm boils down to:
1) I have 2 netdevs
2) One is preferred
3) I setup team on top of them
That's should be it. It is the users responsibility to do it this way.
>
>>>
>>>Essentially this is meant to be a bond that is more-or-less managed by
>>>the host, not the guest. We want the host to be able to configure it
>>
>> How is it managed by the host? In your usecase the guest has 2 netdevs:
>> virtio_net, pci vf.
>> I don't see how host can do any managing of that, other than the
>> obvious. But still, the active/backup decision is done in guest. This is
>> a simple bond/team usecase. As I said, there is something needed to be
>> implemented in userspace in order to handle re-appear of vf netdev.
>> But that should be fairly easy to do in teamd.
>>
>>
>>>and have it automatically kick in on the guest. For now we want to
>>>avoid adding too much complexity as this is meant to be just the first
>>
>> That's what I fear, "for now"..
>
>I used the expression "for now" as I see this being the first stage of
>a multi-stage process.
That is what I fear...
>
>Step 1 is to get a basic virtio-bypass driver added to virtio so that
>it is at least comparable to netvsc in terms of feature set and
>enables basic network live migration.
>
>Step 2 is adding some sort of dirty page tracking, preferably via
>something like a paravirtual iommu interface. Once we have that we can
>defer the eviction of the VF until the very last moment of the live
>migration. For now I need to work on testing a modification to allow
>mapping the entire guest as being pass-through for DMA to the device,
>and requiring dynamic for any DMA that is bidirectional or from the
>device.
That is purely on the host side. Does not really matter if your solution
or standard bond/team is in use, right?
>
>Step 3 will be to start looking at advanced configuration. That is
>where we drop the implementation in step 1 and instead look at
>spawning something that looks more like the team type interface,
>however instead of working with a user-space daemon we would likely
>need to work with some sort of mailbox or message queue coming up from
>the hypervisor. Then we can start looking at doing things like passing
>up blocks of eBPF code to handle Tx port selection or whatever we
>need.
:O
>
>>
>>>step. Trying to go in and implement the whole solution right from the
>>>start based on existing drivers is going to be a massive time sink and
>>>will likely never get completed due to the fact that there is always
>>>going to be some other thing that will interfere.
>>
>> "implement the whole solution right from the start based on existing
>> drivers" - what solution are you talking about? I don't understand this
>> para.
>
>You started mentioning much more complex configurations such as
>multi-VF, LACP, and other such things. I fully own that this cannot
>support that. My understanding is that the netvsc solution that is out
>there cannot support anything like that either. The idea for now is to
>keep this as simple as possible. It makes things like the possibility
>of porting this to other OSes much easier.
Easier solution is team and teamd with linimal modifications in order to
make your usecase work. Btw, do you have the needs for your usecase
written down somewhere, so we are on the same page?
>
>>>
>>>My personal hope is that we can look at doing a virtio-bond sort of
>>>device that will handle all this as well as providing a communication
>>>channel, but that is much further down the road. For now we only have
>>>a single bit so the goal for now is trying to keep this as simple as
>>>possible.
>>
>> Oh. So there is really intention to do re-implementation of bonding
>> in virtio. That is plain-wrong in my opinion.
>>
>> Could you just use bond/team, please, and don't reinvent the wheel with
>> this abomination?
>
>So I have a question for you. Why did you create the team driver? The
>bonding code was already there and does almost exactly the same thing.
Please do go down the git log memory lane. Team was introduced in 2011.
At that time bonding was not in a good shape. I decided to rewrite it
with minimal parts being in kernel to allow the flexibility user needs
to be done in userspace. By the way, the usecase you are trying to
resolve by this patchset is something that can benefit from the team
driver kernel-userspace architecture. Easily.
>I would think it has to do with where things are managed. That is the
>same situation we have with this.
>
>In my mind I don't see this something where we can just fit it into
>one of these two drivers because of the same reason the bonding and
>team drivers are split. We want to manage this interface somewhere
>else. In my mind what we probably need to do is look at refactoring
>the code since the control paths are in different locations for each
>of these drivers, but much of the datapath is the same. That is where
This is where you try to twist the universe, in my opinion. You want to
move the responsibilities of the user inside the guest to the user
ourside it. And you use this weird mechanisms to do so. It feel very
wrong. Look at it from non-virtualized point of view. It's like you
would have HW that would configure the kernel which runs on it. It is
supposed to be the other way around.
>I see things going eventually for this "virtio-bond" interface I
>referenced, but for now this interface is not that since there isn't
>really any communication channel present at all.
That is exactly what I fear of. Thanks for ensuring me that is the final
vision :/
>
>>>
>>>> What is the reason for this abomination? According to:
>>>> https://marc.info/?l=linux-virtualization&m=151189725224231&w=2
>>>> The reason is quite weak.
>>>> User in the vm sees 2 (or more) netdevices, he puts them in bond/team
>>>> and that's it. This works now! If the vm lacks some userspace features,
>>>> let's fix it there! For example the MAC changes is something that could
>>>> be easily handled in teamd userspace deamon.
>>>
>>>I think you might have missed the point of this. This is meant to be a
>>>simple interface so the guest should not be able to change the MAC
>>>address, and it shouldn't require any userspace daemon to setup or
>>>tear down. Ideally with this solution the virtio bypass will come up
>>>and be assigned the name of the original virtio, and the "backup"
>>>interface will come up and be assigned the name of the original virtio
>>>with an additional "nbackup" tacked on via the phys_port_name, and
>>>then whenever a VF is added it will automatically be enslaved by the
>>>bypass interface, and it will be removed when the VF is hotplugged
>>>out.
>>>
>>>In my mind the difference between this and bond or team is where the
>>>configuration interface lies. In the case of bond it is in the kernel.
>>>If my understanding is correct team is mostly in user space. With this
>>>the configuration interface is really down in the hypervisor and
>>>requests are communicated up to the guest. I would prefer not to make
>>>virtio_net dependent on the bonding or team drivers, or worse yet a
>>>userspace daemon in the guest. For now I would argue we should keep
>>>this as simple as possible just to support basic live migration. There
>>>has already been discussions of refactoring this after it is in so
>>>that we can start to combine the functionality here with what is there
>>>in bonding/team, but the differences in configuration interface and
>>>the size of the code bases will make it challenging to outright merge
>>>this into something like that.
^ permalink raw reply
* Re: [RFC PATCH v3 0/3] Enable virtio_net to act as a backup for a passthru device
From: Alexander Duyck @ 2018-02-20 17:23 UTC (permalink / raw)
To: Jiri Pirko
Cc: Duyck, Alexander H, virtio-dev, Michael S. Tsirkin,
Jakub Kicinski, Sridhar Samudrala, virtualization, Siwei Liu,
Netdev, David Miller
In-Reply-To: <20180220162933.GD2031@nanopsycho>
On Tue, Feb 20, 2018 at 8:29 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Tue, Feb 20, 2018 at 05:04:29PM CET, alexander.duyck@gmail.com wrote:
>>On Tue, Feb 20, 2018 at 2:42 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> Fri, Feb 16, 2018 at 07:11:19PM CET, sridhar.samudrala@intel.com wrote:
>>>>Patch 1 introduces a new feature bit VIRTIO_NET_F_BACKUP that can be
>>>>used by hypervisor to indicate that virtio_net interface should act as
>>>>a backup for another device with the same MAC address.
>>>>
>>>>Ppatch 2 is in response to the community request for a 3 netdev
>>>>solution. However, it creates some issues we'll get into in a moment.
>>>>It extends virtio_net to use alternate datapath when available and
>>>>registered. When BACKUP feature is enabled, virtio_net driver creates
>>>>an additional 'bypass' netdev that acts as a master device and controls
>>>>2 slave devices. The original virtio_net netdev is registered as
>>>>'backup' netdev and a passthru/vf device with the same MAC gets
>>>>registered as 'active' netdev. Both 'bypass' and 'backup' netdevs are
>>>>associated with the same 'pci' device. The user accesses the network
>>>>interface via 'bypass' netdev. The 'bypass' netdev chooses 'active' netdev
>>>>as default for transmits when it is available with link up and running.
>>>
>>> Sorry, but this is ridiculous. You are apparently re-implemeting part
>>> of bonding driver as a part of NIC driver. Bond and team drivers
>>> are mature solutions, well tested, broadly used, with lots of issues
>>> resolved in the past. What you try to introduce is a weird shortcut
>>> that already has couple of issues as you mentioned and will certanly
>>> have many more. Also, I'm pretty sure that in future, someone comes up
>>> with ideas like multiple VFs, LACP and similar bonding things.
>>
>>The problem with the bond and team drivers is they are too large and
>>have too many interfaces available for configuration so as a result
>>they can really screw this interface up.
>
> What? Too large is which sense? Why "too many interfaces" is a problem?
> Also, team has only one interface to userspace team-generic-netlink.
Specifically I was working with bond. I had overlooked team for the
most part since it required an additional userspace daemon which
basically broke our requirement of no user-space intervention.
I was trying to focus on just doing an active/backup setup. The
problem is there are debugfs, sysfs, and procfs interfaces exposed
that we don't need and/or want. Adding any sort of interface to
exclude these would just bloat up the bonding driver, and leaving them
in would just be confusing since they would all need to be ignored. In
addition the steps needed to get the name to come out the same as the
original virtio interface would just bloat up bonding.
>>
>>Essentially this is meant to be a bond that is more-or-less managed by
>>the host, not the guest. We want the host to be able to configure it
>
> How is it managed by the host? In your usecase the guest has 2 netdevs:
> virtio_net, pci vf.
> I don't see how host can do any managing of that, other than the
> obvious. But still, the active/backup decision is done in guest. This is
> a simple bond/team usecase. As I said, there is something needed to be
> implemented in userspace in order to handle re-appear of vf netdev.
> But that should be fairly easy to do in teamd.
>
>
>>and have it automatically kick in on the guest. For now we want to
>>avoid adding too much complexity as this is meant to be just the first
>
> That's what I fear, "for now"..
I used the expression "for now" as I see this being the first stage of
a multi-stage process.
Step 1 is to get a basic virtio-bypass driver added to virtio so that
it is at least comparable to netvsc in terms of feature set and
enables basic network live migration.
Step 2 is adding some sort of dirty page tracking, preferably via
something like a paravirtual iommu interface. Once we have that we can
defer the eviction of the VF until the very last moment of the live
migration. For now I need to work on testing a modification to allow
mapping the entire guest as being pass-through for DMA to the device,
and requiring dynamic for any DMA that is bidirectional or from the
device.
Step 3 will be to start looking at advanced configuration. That is
where we drop the implementation in step 1 and instead look at
spawning something that looks more like the team type interface,
however instead of working with a user-space daemon we would likely
need to work with some sort of mailbox or message queue coming up from
the hypervisor. Then we can start looking at doing things like passing
up blocks of eBPF code to handle Tx port selection or whatever we
need.
>
>>step. Trying to go in and implement the whole solution right from the
>>start based on existing drivers is going to be a massive time sink and
>>will likely never get completed due to the fact that there is always
>>going to be some other thing that will interfere.
>
> "implement the whole solution right from the start based on existing
> drivers" - what solution are you talking about? I don't understand this
> para.
You started mentioning much more complex configurations such as
multi-VF, LACP, and other such things. I fully own that this cannot
support that. My understanding is that the netvsc solution that is out
there cannot support anything like that either. The idea for now is to
keep this as simple as possible. It makes things like the possibility
of porting this to other OSes much easier.
>>
>>My personal hope is that we can look at doing a virtio-bond sort of
>>device that will handle all this as well as providing a communication
>>channel, but that is much further down the road. For now we only have
>>a single bit so the goal for now is trying to keep this as simple as
>>possible.
>
> Oh. So there is really intention to do re-implementation of bonding
> in virtio. That is plain-wrong in my opinion.
>
> Could you just use bond/team, please, and don't reinvent the wheel with
> this abomination?
So I have a question for you. Why did you create the team driver? The
bonding code was already there and does almost exactly the same thing.
I would think it has to do with where things are managed. That is the
same situation we have with this.
In my mind I don't see this something where we can just fit it into
one of these two drivers because of the same reason the bonding and
team drivers are split. We want to manage this interface somewhere
else. In my mind what we probably need to do is look at refactoring
the code since the control paths are in different locations for each
of these drivers, but much of the datapath is the same. That is where
I see things going eventually for this "virtio-bond" interface I
referenced, but for now this interface is not that since there isn't
really any communication channel present at all.
>>
>>> What is the reason for this abomination? According to:
>>> https://marc.info/?l=linux-virtualization&m=151189725224231&w=2
>>> The reason is quite weak.
>>> User in the vm sees 2 (or more) netdevices, he puts them in bond/team
>>> and that's it. This works now! If the vm lacks some userspace features,
>>> let's fix it there! For example the MAC changes is something that could
>>> be easily handled in teamd userspace deamon.
>>
>>I think you might have missed the point of this. This is meant to be a
>>simple interface so the guest should not be able to change the MAC
>>address, and it shouldn't require any userspace daemon to setup or
>>tear down. Ideally with this solution the virtio bypass will come up
>>and be assigned the name of the original virtio, and the "backup"
>>interface will come up and be assigned the name of the original virtio
>>with an additional "nbackup" tacked on via the phys_port_name, and
>>then whenever a VF is added it will automatically be enslaved by the
>>bypass interface, and it will be removed when the VF is hotplugged
>>out.
>>
>>In my mind the difference between this and bond or team is where the
>>configuration interface lies. In the case of bond it is in the kernel.
>>If my understanding is correct team is mostly in user space. With this
>>the configuration interface is really down in the hypervisor and
>>requests are communicated up to the guest. I would prefer not to make
>>virtio_net dependent on the bonding or team drivers, or worse yet a
>>userspace daemon in the guest. For now I would argue we should keep
>>this as simple as possible just to support basic live migration. There
>>has already been discussions of refactoring this after it is in so
>>that we can start to combine the functionality here with what is there
>>in bonding/team, but the differences in configuration interface and
>>the size of the code bases will make it challenging to outright merge
>>this into something like that.
^ permalink raw reply
* Re: [RFC PATCH v3 0/3] Enable virtio_net to act as a backup for a passthru device
From: Samudrala, Sridhar @ 2018-02-20 17:14 UTC (permalink / raw)
To: Jiri Pirko, Alexander Duyck
Cc: Duyck, Alexander H, virtio-dev, Michael S. Tsirkin,
Jakub Kicinski, Netdev, virtualization, Siwei Liu, David Miller
In-Reply-To: <20180220162933.GD2031@nanopsycho>
On 2/20/2018 8:29 AM, Jiri Pirko wrote:
> Tue, Feb 20, 2018 at 05:04:29PM CET, alexander.duyck@gmail.com wrote:
>> On Tue, Feb 20, 2018 at 2:42 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> Fri, Feb 16, 2018 at 07:11:19PM CET, sridhar.samudrala@intel.com wrote:
>>>> Patch 1 introduces a new feature bit VIRTIO_NET_F_BACKUP that can be
>>>> used by hypervisor to indicate that virtio_net interface should act as
>>>> a backup for another device with the same MAC address.
>>>>
>>>> Ppatch 2 is in response to the community request for a 3 netdev
>>>> solution. However, it creates some issues we'll get into in a moment.
>>>> It extends virtio_net to use alternate datapath when available and
>>>> registered. When BACKUP feature is enabled, virtio_net driver creates
>>>> an additional 'bypass' netdev that acts as a master device and controls
>>>> 2 slave devices. The original virtio_net netdev is registered as
>>>> 'backup' netdev and a passthru/vf device with the same MAC gets
>>>> registered as 'active' netdev. Both 'bypass' and 'backup' netdevs are
>>>> associated with the same 'pci' device. The user accesses the network
>>>> interface via 'bypass' netdev. The 'bypass' netdev chooses 'active' netdev
>>>> as default for transmits when it is available with link up and running.
>>> Sorry, but this is ridiculous. You are apparently re-implemeting part
>>> of bonding driver as a part of NIC driver. Bond and team drivers
>>> are mature solutions, well tested, broadly used, with lots of issues
>>> resolved in the past. What you try to introduce is a weird shortcut
>>> that already has couple of issues as you mentioned and will certanly
>>> have many more. Also, I'm pretty sure that in future, someone comes up
>>> with ideas like multiple VFs, LACP and similar bonding things.
>> The problem with the bond and team drivers is they are too large and
>> have too many interfaces available for configuration so as a result
>> they can really screw this interface up.
> What? Too large is which sense? Why "too many interfaces" is a problem?
> Also, team has only one interface to userspace team-generic-netlink.
>
>
>> Essentially this is meant to be a bond that is more-or-less managed by
>> the host, not the guest. We want the host to be able to configure it
> How is it managed by the host? In your usecase the guest has 2 netdevs:
> virtio_net, pci vf.
> I don't see how host can do any managing of that, other than the
> obvious. But still, the active/backup decision is done in guest. This is
> a simple bond/team usecase. As I said, there is something needed to be
> implemented in userspace in order to handle re-appear of vf netdev.
> But that should be fairly easy to do in teamd.
The host manages the active/backup decision by
- assigning the same MAC address to both VF and virtio interfaces
- setting a BACKUP feature bit on virtio that enables virtio to
transparently take
over the VFs datapath.
- only enable one datapath at anytime so that packets don't get looped back
- during live migration enable virtio datapth, unplug vf on the source
and replug
vf on the destination.
The VM is not expected and doesn't have any control of setting the MAC
address
or bringing up/down the links.
This is the model that is currently supported with netvsc driver on Azure.
>
>
>> and have it automatically kick in on the guest. For now we want to
>> avoid adding too much complexity as this is meant to be just the first
> That's what I fear, "for now"..
>
>
>> step. Trying to go in and implement the whole solution right from the
>> start based on existing drivers is going to be a massive time sink and
>> will likely never get completed due to the fact that there is always
>> going to be some other thing that will interfere.
> "implement the whole solution right from the start based on existing
> drivers" - what solution are you talking about? I don't understand this
> para.
>
>
>> My personal hope is that we can look at doing a virtio-bond sort of
>> device that will handle all this as well as providing a communication
>> channel, but that is much further down the road. For now we only have
>> a single bit so the goal for now is trying to keep this as simple as
>> possible.
> Oh. So there is really intention to do re-implementation of bonding
> in virtio. That is plain-wrong in my opinion.
>
> Could you just use bond/team, please, and don't reinvent the wheel with
> this abomination?
>
>>> What is the reason for this abomination? According to:
>>> https://marc.info/?l=linux-virtualization&m=151189725224231&w=2
>>> The reason is quite weak.
>>> User in the vm sees 2 (or more) netdevices, he puts them in bond/team
>>> and that's it. This works now! If the vm lacks some userspace features,
>>> let's fix it there! For example the MAC changes is something that could
>>> be easily handled in teamd userspace deamon.
>> I think you might have missed the point of this. This is meant to be a
>> simple interface so the guest should not be able to change the MAC
>> address, and it shouldn't require any userspace daemon to setup or
>> tear down. Ideally with this solution the virtio bypass will come up
>> and be assigned the name of the original virtio, and the "backup"
>> interface will come up and be assigned the name of the original virtio
>> with an additional "nbackup" tacked on via the phys_port_name, and
>> then whenever a VF is added it will automatically be enslaved by the
>> bypass interface, and it will be removed when the VF is hotplugged
>> out.
>>
>> In my mind the difference between this and bond or team is where the
>> configuration interface lies. In the case of bond it is in the kernel.
>> If my understanding is correct team is mostly in user space. With this
>> the configuration interface is really down in the hypervisor and
>> requests are communicated up to the guest. I would prefer not to make
>> virtio_net dependent on the bonding or team drivers, or worse yet a
>> userspace daemon in the guest. For now I would argue we should keep
>> this as simple as possible just to support basic live migration. There
>> has already been discussions of refactoring this after it is in so
>> that we can start to combine the functionality here with what is there
>> in bonding/team, but the differences in configuration interface and
>> the size of the code bases will make it challenging to outright merge
>> this into something like that.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH v3 0/3] Enable virtio_net to act as a backup for a passthru device
From: Jiri Pirko @ 2018-02-20 16:29 UTC (permalink / raw)
To: Alexander Duyck
Cc: Duyck, Alexander H, virtio-dev, Michael S. Tsirkin,
Jakub Kicinski, Sridhar Samudrala, virtualization, Siwei Liu,
Netdev, David Miller
In-Reply-To: <CAKgT0UdU8PDXduzxp4kKfur-DLeFQSJj7-fhW_eTgVzd+AcViw@mail.gmail.com>
Tue, Feb 20, 2018 at 05:04:29PM CET, alexander.duyck@gmail.com wrote:
>On Tue, Feb 20, 2018 at 2:42 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Fri, Feb 16, 2018 at 07:11:19PM CET, sridhar.samudrala@intel.com wrote:
>>>Patch 1 introduces a new feature bit VIRTIO_NET_F_BACKUP that can be
>>>used by hypervisor to indicate that virtio_net interface should act as
>>>a backup for another device with the same MAC address.
>>>
>>>Ppatch 2 is in response to the community request for a 3 netdev
>>>solution. However, it creates some issues we'll get into in a moment.
>>>It extends virtio_net to use alternate datapath when available and
>>>registered. When BACKUP feature is enabled, virtio_net driver creates
>>>an additional 'bypass' netdev that acts as a master device and controls
>>>2 slave devices. The original virtio_net netdev is registered as
>>>'backup' netdev and a passthru/vf device with the same MAC gets
>>>registered as 'active' netdev. Both 'bypass' and 'backup' netdevs are
>>>associated with the same 'pci' device. The user accesses the network
>>>interface via 'bypass' netdev. The 'bypass' netdev chooses 'active' netdev
>>>as default for transmits when it is available with link up and running.
>>
>> Sorry, but this is ridiculous. You are apparently re-implemeting part
>> of bonding driver as a part of NIC driver. Bond and team drivers
>> are mature solutions, well tested, broadly used, with lots of issues
>> resolved in the past. What you try to introduce is a weird shortcut
>> that already has couple of issues as you mentioned and will certanly
>> have many more. Also, I'm pretty sure that in future, someone comes up
>> with ideas like multiple VFs, LACP and similar bonding things.
>
>The problem with the bond and team drivers is they are too large and
>have too many interfaces available for configuration so as a result
>they can really screw this interface up.
What? Too large is which sense? Why "too many interfaces" is a problem?
Also, team has only one interface to userspace team-generic-netlink.
>
>Essentially this is meant to be a bond that is more-or-less managed by
>the host, not the guest. We want the host to be able to configure it
How is it managed by the host? In your usecase the guest has 2 netdevs:
virtio_net, pci vf.
I don't see how host can do any managing of that, other than the
obvious. But still, the active/backup decision is done in guest. This is
a simple bond/team usecase. As I said, there is something needed to be
implemented in userspace in order to handle re-appear of vf netdev.
But that should be fairly easy to do in teamd.
>and have it automatically kick in on the guest. For now we want to
>avoid adding too much complexity as this is meant to be just the first
That's what I fear, "for now"..
>step. Trying to go in and implement the whole solution right from the
>start based on existing drivers is going to be a massive time sink and
>will likely never get completed due to the fact that there is always
>going to be some other thing that will interfere.
"implement the whole solution right from the start based on existing
drivers" - what solution are you talking about? I don't understand this
para.
>
>My personal hope is that we can look at doing a virtio-bond sort of
>device that will handle all this as well as providing a communication
>channel, but that is much further down the road. For now we only have
>a single bit so the goal for now is trying to keep this as simple as
>possible.
Oh. So there is really intention to do re-implementation of bonding
in virtio. That is plain-wrong in my opinion.
Could you just use bond/team, please, and don't reinvent the wheel with
this abomination?
>
>> What is the reason for this abomination? According to:
>> https://marc.info/?l=linux-virtualization&m=151189725224231&w=2
>> The reason is quite weak.
>> User in the vm sees 2 (or more) netdevices, he puts them in bond/team
>> and that's it. This works now! If the vm lacks some userspace features,
>> let's fix it there! For example the MAC changes is something that could
>> be easily handled in teamd userspace deamon.
>
>I think you might have missed the point of this. This is meant to be a
>simple interface so the guest should not be able to change the MAC
>address, and it shouldn't require any userspace daemon to setup or
>tear down. Ideally with this solution the virtio bypass will come up
>and be assigned the name of the original virtio, and the "backup"
>interface will come up and be assigned the name of the original virtio
>with an additional "nbackup" tacked on via the phys_port_name, and
>then whenever a VF is added it will automatically be enslaved by the
>bypass interface, and it will be removed when the VF is hotplugged
>out.
>
>In my mind the difference between this and bond or team is where the
>configuration interface lies. In the case of bond it is in the kernel.
>If my understanding is correct team is mostly in user space. With this
>the configuration interface is really down in the hypervisor and
>requests are communicated up to the guest. I would prefer not to make
>virtio_net dependent on the bonding or team drivers, or worse yet a
>userspace daemon in the guest. For now I would argue we should keep
>this as simple as possible just to support basic live migration. There
>has already been discussions of refactoring this after it is in so
>that we can start to combine the functionality here with what is there
>in bonding/team, but the differences in configuration interface and
>the size of the code bases will make it challenging to outright merge
>this into something like that.
^ permalink raw reply
* Re: [RFC PATCH v3 0/3] Enable virtio_net to act as a backup for a passthru device
From: Samudrala, Sridhar @ 2018-02-20 16:26 UTC (permalink / raw)
To: Jakub Kicinski, Alexander Duyck
Cc: Duyck, Alexander H, virtio-dev, Michael S. Tsirkin, Netdev,
virtualization, Siwei Liu, David Miller
In-Reply-To: <20180218221140.6c7fb32e@cakuba.netronome.com>
On 2/18/2018 10:11 PM, Jakub Kicinski wrote:
> On Sat, 17 Feb 2018 09:12:01 -0800, Alexander Duyck wrote:
>>>> We noticed a couple of issues with this approach during testing.
>>>> - As both 'bypass' and 'backup' netdevs are associated with the same
>>>> virtio pci device, udev tries to rename both of them with the same name
>>>> and the 2nd rename will fail. This would be OK as long as the first netdev
>>>> to be renamed is the 'bypass' netdev, but the order in which udev gets
>>>> to rename the 2 netdevs is not reliable.
>>> Out of curiosity - why do you link the master netdev to the virtio
>>> struct device?
>> The basic idea of all this is that we wanted this to work with an
>> existing VM image that was using virtio. As such we were trying to
>> make it so that the bypass interface takes the place of the original
>> virtio and get udev to rename the bypass to what the original
>> virtio_net was.
> That makes sense. Is it udev/naming that you're most concerned about
> here? I.e. what's the user space app that expects the netdev to be
> linked? This is just out of curiosity, the linking of netdevs to
> devices is a bit of a PITA in the switchdev eswitch mode world, with
> libvirt expecting only certain devices to be there.. Right now we're
> not linking VF reprs, which breaks naming. I wanted to revisit that.
For live migration usecase, userspace is only aware of one virtio_net
interface and
it doesn't expect it to be linked with any lower dev. So it should be
fine even if the
lower netdev is not present. Only the master netdev should be assigned
the same
name so that userspace configuration scripts in the VM don't need to
change.
>
>>> FWIW two solutions that immediately come to mind is to export "backup"
>>> as phys_port_name of the backup virtio link and/or assign a name to the
>>> master like you are doing already. I think team uses team%d and bond
>>> uses bond%d, soft naming of master devices seems quite natural in this
>>> case.
>> I figured I had overlooked something like that.. Thanks for pointing
>> this out. Okay so I think the phys_port_name approach might resolve
>> the original issue. If I am reading things correctly what we end up
>> with is the master showing up as "ens1" for example and the backup
>> showing up as "ens1nbackup". Am I understanding that right?
> Yes, provided systemd is new enough.
Yes. I did a quick test to confirm that adding ndo_phys_port_name() to
virtio_net
ndo_ops fixes the udev naming issue with 2 virtio netdevs. This is on
fedora27.
>
>> The problem with the team/bond%d approach is that it creates a new
>> netdevice and so it would require guest configuration changes.
>>
>>> IMHO phys_port_name == "backup" if BACKUP bit is set on slave virtio
>>> link is quite neat.
>> I agree. For non-"backup" virio_net devices would it be okay for us to
>> just return -EOPNOTSUPP? I assume it would be and that way the legacy
>> behavior could be maintained although the function still exists.
> That's my understanding too.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH v3 0/3] Enable virtio_net to act as a backup for a passthru device
From: Alexander Duyck @ 2018-02-20 16:04 UTC (permalink / raw)
To: Jiri Pirko
Cc: Duyck, Alexander H, virtio-dev, Michael S. Tsirkin,
Jakub Kicinski, Sridhar Samudrala, virtualization, Siwei Liu,
Netdev, David Miller
In-Reply-To: <20180220104224.GA2031@nanopsycho>
On Tue, Feb 20, 2018 at 2:42 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Fri, Feb 16, 2018 at 07:11:19PM CET, sridhar.samudrala@intel.com wrote:
>>Patch 1 introduces a new feature bit VIRTIO_NET_F_BACKUP that can be
>>used by hypervisor to indicate that virtio_net interface should act as
>>a backup for another device with the same MAC address.
>>
>>Ppatch 2 is in response to the community request for a 3 netdev
>>solution. However, it creates some issues we'll get into in a moment.
>>It extends virtio_net to use alternate datapath when available and
>>registered. When BACKUP feature is enabled, virtio_net driver creates
>>an additional 'bypass' netdev that acts as a master device and controls
>>2 slave devices. The original virtio_net netdev is registered as
>>'backup' netdev and a passthru/vf device with the same MAC gets
>>registered as 'active' netdev. Both 'bypass' and 'backup' netdevs are
>>associated with the same 'pci' device. The user accesses the network
>>interface via 'bypass' netdev. The 'bypass' netdev chooses 'active' netdev
>>as default for transmits when it is available with link up and running.
>
> Sorry, but this is ridiculous. You are apparently re-implemeting part
> of bonding driver as a part of NIC driver. Bond and team drivers
> are mature solutions, well tested, broadly used, with lots of issues
> resolved in the past. What you try to introduce is a weird shortcut
> that already has couple of issues as you mentioned and will certanly
> have many more. Also, I'm pretty sure that in future, someone comes up
> with ideas like multiple VFs, LACP and similar bonding things.
The problem with the bond and team drivers is they are too large and
have too many interfaces available for configuration so as a result
they can really screw this interface up.
Essentially this is meant to be a bond that is more-or-less managed by
the host, not the guest. We want the host to be able to configure it
and have it automatically kick in on the guest. For now we want to
avoid adding too much complexity as this is meant to be just the first
step. Trying to go in and implement the whole solution right from the
start based on existing drivers is going to be a massive time sink and
will likely never get completed due to the fact that there is always
going to be some other thing that will interfere.
My personal hope is that we can look at doing a virtio-bond sort of
device that will handle all this as well as providing a communication
channel, but that is much further down the road. For now we only have
a single bit so the goal for now is trying to keep this as simple as
possible.
> What is the reason for this abomination? According to:
> https://marc.info/?l=linux-virtualization&m=151189725224231&w=2
> The reason is quite weak.
> User in the vm sees 2 (or more) netdevices, he puts them in bond/team
> and that's it. This works now! If the vm lacks some userspace features,
> let's fix it there! For example the MAC changes is something that could
> be easily handled in teamd userspace deamon.
I think you might have missed the point of this. This is meant to be a
simple interface so the guest should not be able to change the MAC
address, and it shouldn't require any userspace daemon to setup or
tear down. Ideally with this solution the virtio bypass will come up
and be assigned the name of the original virtio, and the "backup"
interface will come up and be assigned the name of the original virtio
with an additional "nbackup" tacked on via the phys_port_name, and
then whenever a VF is added it will automatically be enslaved by the
bypass interface, and it will be removed when the VF is hotplugged
out.
In my mind the difference between this and bond or team is where the
configuration interface lies. In the case of bond it is in the kernel.
If my understanding is correct team is mostly in user space. With this
the configuration interface is really down in the hypervisor and
requests are communicated up to the guest. I would prefer not to make
virtio_net dependent on the bonding or team drivers, or worse yet a
userspace daemon in the guest. For now I would argue we should keep
this as simple as possible just to support basic live migration. There
has already been discussions of refactoring this after it is in so
that we can start to combine the functionality here with what is there
in bonding/team, but the differences in configuration interface and
the size of the code bases will make it challenging to outright merge
this into something like that.
^ permalink raw reply
* [4.4-stable 12/22] virtio_balloon: prevent uninitialized variable use
From: Arnd Bergmann @ 2018-02-20 11:54 UTC (permalink / raw)
To: stable
Cc: Arnd Bergmann, Michael S . Tsirkin, Greg KH, Ladi Prosek,
linux-kernel, virtualization
In-Reply-To: <20180220115527.1806578-1-arnd@arndb.de>
commit f0bb2d50dfcc519f06f901aac88502be6ff1df2c upstream.
The latest gcc-7.0.1 snapshot reports a new warning:
virtio/virtio_balloon.c: In function 'update_balloon_stats':
virtio/virtio_balloon.c:258:26: error: 'events[2]' is used uninitialized in this function [-Werror=uninitialized]
virtio/virtio_balloon.c:260:26: error: 'events[3]' is used uninitialized in this function [-Werror=uninitialized]
virtio/virtio_balloon.c:261:56: error: 'events[18]' is used uninitialized in this function [-Werror=uninitialized]
virtio/virtio_balloon.c:262:56: error: 'events[17]' is used uninitialized in this function [-Werror=uninitialized]
This seems absolutely right, so we should add an extra check to
prevent copying uninitialized stack data into the statistics.
From all I can tell, this has been broken since the statistics code
was originally added in 2.6.34.
Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon driver (V4)")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Ladi Prosek <lprosek@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
[arnd: backported to 4.4]
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/virtio/virtio_balloon.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 01d15dca940e..26d0dff069f0 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -239,12 +239,15 @@ static void update_balloon_stats(struct virtio_balloon *vb)
all_vm_events(events);
si_meminfo(&i);
+
+#ifdef CONFIG_VM_EVENT_COUNTERS
update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
pages_to_bytes(events[PSWPIN]));
update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
pages_to_bytes(events[PSWPOUT]));
update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
+#endif
update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
pages_to_bytes(i.freeram));
update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,
--
2.9.0
^ permalink raw reply related
* Re: [PATCH 1/4] iommu: Add virtio-iommu driver
From: Jean-Philippe Brucker @ 2018-02-20 11:30 UTC (permalink / raw)
To: Tomasz Nowicki, iommu@lists.linux-foundation.org,
kvm@vger.kernel.org, virtualization@lists.linux-foundation.org,
virtio-dev@lists.oasis-open.org, kvmarm@lists.cs.columbia.edu
Cc: jayachandran.nair@cavium.com, Lorenzo Pieralisi,
tnowicki@caviumnetworks.com, mst@redhat.com, Marc Zyngier,
Will Deacon, jintack@cs.columbia.edu, eric.auger@redhat.com,
Robin Murphy, joro@8bytes.org, eric.auger.pro@gmail.com
In-Reply-To: <417e1617-e5ff-7b13-64e3-a8a98d30bcb9@semihalf.com>
On 19/02/18 12:23, Tomasz Nowicki wrote:
[...]
>> +static int viommu_receive_resp(struct viommu_dev *viommu, int nr_sent,
>> + struct list_head *sent)
>> +{
>> +
>> + unsigned int len;
>> + int nr_received = 0;
>> + struct viommu_request *req, *pending;
>> +
>> + pending = list_first_entry_or_null(sent, struct viommu_request, list);
>> + if (WARN_ON(!pending))
>> + return 0;
>> +
>> + while ((req = virtqueue_get_buf(viommu->vq, &len)) != NULL) {
>> + if (req != pending) {
>> + dev_warn(viommu->dev, "discarding stale request\n");
>> + continue;
>> + }
>> +
>> + pending->written = len;
>> +
>> + if (++nr_received == nr_sent) {
>> + WARN_ON(!list_is_last(&pending->list, sent));
>> + break;
>> + } else if (WARN_ON(list_is_last(&pending->list, sent))) {
>> + break;
>> + }
>> +
>> + pending = list_next_entry(pending, list);
>
> We should remove current element from the pending list. There is no
> guarantee we get response for each while loop so when we get back for
> more the _viommu_send_reqs_sync() caller will pass pointer to the out of
> date head next time.
Right, I'll fix this
Thanks,
Jean
^ permalink raw reply
* Re: [RFC PATCH v3 0/3] Enable virtio_net to act as a backup for a passthru device
From: Jiri Pirko @ 2018-02-20 10:42 UTC (permalink / raw)
To: Sridhar Samudrala
Cc: alexander.h.duyck, virtio-dev, mst, kubakici, netdev,
virtualization, loseweigh, davem
In-Reply-To: <1518804682-16881-1-git-send-email-sridhar.samudrala@intel.com>
Fri, Feb 16, 2018 at 07:11:19PM CET, sridhar.samudrala@intel.com wrote:
>Patch 1 introduces a new feature bit VIRTIO_NET_F_BACKUP that can be
>used by hypervisor to indicate that virtio_net interface should act as
>a backup for another device with the same MAC address.
>
>Ppatch 2 is in response to the community request for a 3 netdev
>solution. However, it creates some issues we'll get into in a moment.
>It extends virtio_net to use alternate datapath when available and
>registered. When BACKUP feature is enabled, virtio_net driver creates
>an additional 'bypass' netdev that acts as a master device and controls
>2 slave devices. The original virtio_net netdev is registered as
>'backup' netdev and a passthru/vf device with the same MAC gets
>registered as 'active' netdev. Both 'bypass' and 'backup' netdevs are
>associated with the same 'pci' device. The user accesses the network
>interface via 'bypass' netdev. The 'bypass' netdev chooses 'active' netdev
>as default for transmits when it is available with link up and running.
Sorry, but this is ridiculous. You are apparently re-implemeting part
of bonding driver as a part of NIC driver. Bond and team drivers
are mature solutions, well tested, broadly used, with lots of issues
resolved in the past. What you try to introduce is a weird shortcut
that already has couple of issues as you mentioned and will certanly
have many more. Also, I'm pretty sure that in future, someone comes up
with ideas like multiple VFs, LACP and similar bonding things.
What is the reason for this abomination? According to:
https://marc.info/?l=linux-virtualization&m=151189725224231&w=2
The reason is quite weak.
User in the vm sees 2 (or more) netdevices, he puts them in bond/team
and that's it. This works now! If the vm lacks some userspace features,
let's fix it there! For example the MAC changes is something that could
be easily handled in teamd userspace deamon.
^ permalink raw reply
* Re: [PATCH] drm/bochs: make structure bochs_bo_driver static
From: Daniel Vetter @ 2018-02-19 9:43 UTC (permalink / raw)
To: Colin King
Cc: David Airlie, kernel-janitors, linux-kernel, dri-devel,
virtualization
In-Reply-To: <20180207111353.30261-1-colin.king@canonical.com>
On Wed, Feb 07, 2018 at 11:13:53AM +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The structure bochs_bo_driver is local to the source and does not need to
> be in global scope, so make it static.
>
> Cleans up sparse warning:
> drivers/gpu/drm/bochs/bochs_mm.c:197:22: warning: symbol 'bochs_bo_driver'
> was not declared. Should it be static?
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Applied to drm-misc-next, thanks.
-Daniel
> ---
> drivers/gpu/drm/bochs/bochs_mm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c
> index 704e879711e4..b1d5aee46316 100644
> --- a/drivers/gpu/drm/bochs/bochs_mm.c
> +++ b/drivers/gpu/drm/bochs/bochs_mm.c
> @@ -194,7 +194,7 @@ static struct ttm_tt *bochs_ttm_tt_create(struct ttm_bo_device *bdev,
> return tt;
> }
>
> -struct ttm_bo_driver bochs_bo_driver = {
> +static struct ttm_bo_driver bochs_bo_driver = {
> .ttm_tt_create = bochs_ttm_tt_create,
> .ttm_tt_populate = ttm_pool_populate,
> .ttm_tt_unpopulate = ttm_pool_unpopulate,
> --
> 2.15.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply
* Re: [RFC PATCH v3 0/3] Enable virtio_net to act as a backup for a passthru device
From: Jakub Kicinski @ 2018-02-19 6:11 UTC (permalink / raw)
To: Alexander Duyck
Cc: Duyck, Alexander H, virtio-dev, Michael S. Tsirkin,
Sridhar Samudrala, virtualization, Siwei Liu, Netdev,
David Miller
In-Reply-To: <CAKgT0UfU9gV2Y7hWFQi0vJoD9kYeoq+7cXmwAjXeV5zxXHa3dQ@mail.gmail.com>
On Sat, 17 Feb 2018 09:12:01 -0800, Alexander Duyck wrote:
> >> We noticed a couple of issues with this approach during testing.
> >> - As both 'bypass' and 'backup' netdevs are associated with the same
> >> virtio pci device, udev tries to rename both of them with the same name
> >> and the 2nd rename will fail. This would be OK as long as the first netdev
> >> to be renamed is the 'bypass' netdev, but the order in which udev gets
> >> to rename the 2 netdevs is not reliable.
> >
> > Out of curiosity - why do you link the master netdev to the virtio
> > struct device?
>
> The basic idea of all this is that we wanted this to work with an
> existing VM image that was using virtio. As such we were trying to
> make it so that the bypass interface takes the place of the original
> virtio and get udev to rename the bypass to what the original
> virtio_net was.
That makes sense. Is it udev/naming that you're most concerned about
here? I.e. what's the user space app that expects the netdev to be
linked? This is just out of curiosity, the linking of netdevs to
devices is a bit of a PITA in the switchdev eswitch mode world, with
libvirt expecting only certain devices to be there.. Right now we're
not linking VF reprs, which breaks naming. I wanted to revisit that.
> > FWIW two solutions that immediately come to mind is to export "backup"
> > as phys_port_name of the backup virtio link and/or assign a name to the
> > master like you are doing already. I think team uses team%d and bond
> > uses bond%d, soft naming of master devices seems quite natural in this
> > case.
>
> I figured I had overlooked something like that.. Thanks for pointing
> this out. Okay so I think the phys_port_name approach might resolve
> the original issue. If I am reading things correctly what we end up
> with is the master showing up as "ens1" for example and the backup
> showing up as "ens1nbackup". Am I understanding that right?
Yes, provided systemd is new enough.
> The problem with the team/bond%d approach is that it creates a new
> netdevice and so it would require guest configuration changes.
>
> > IMHO phys_port_name == "backup" if BACKUP bit is set on slave virtio
> > link is quite neat.
>
> I agree. For non-"backup" virio_net devices would it be okay for us to
> just return -EOPNOTSUPP? I assume it would be and that way the legacy
> behavior could be maintained although the function still exists.
That's my understanding too.
^ permalink raw reply
* Re: [RFC PATCH v3 2/3] virtio_net: Extend virtio to use VF datapath when available
From: Alexander Duyck @ 2018-02-17 17:41 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Duyck, Alexander H, Or Gerlitz, Michael S. Tsirkin,
Sridhar Samudrala, virtualization, Siwei Liu, Netdev,
David Miller
In-Reply-To: <20180216190455.2a0ea023@cakuba.netronome.com>
On Fri, Feb 16, 2018 at 7:04 PM, Jakub Kicinski <kubakici@wp.pl> wrote:
> On Fri, 16 Feb 2018 10:11:21 -0800, Sridhar Samudrala wrote:
>> This patch enables virtio_net to switch over to a VF datapath when a VF
>> netdev is present with the same MAC address. It allows live migration
>> of a VM with a direct attached VF without the need to setup a bond/team
>> between a VF and virtio net device in the guest.
>>
>> The hypervisor needs to enable only one datapath at any time so that
>> packets don't get looped back to the VM over the other datapath. When a VF
>> is plugged, the virtio datapath link state can be marked as down. The
>> hypervisor needs to unplug the VF device from the guest on the source host
>> and reset the MAC filter of the VF to initiate failover of datapath to
>> virtio before starting the migration. After the migration is completed,
>> the destination hypervisor sets the MAC filter on the VF and plugs it back
>> to the guest to switch over to VF datapath.
>>
>> When BACKUP feature is enabled, an additional netdev(bypass netdev) is
>> created that acts as a master device and tracks the state of the 2 lower
>> netdevs. The original virtio_net netdev is marked as 'backup' netdev and a
>> passthru device with the same MAC is registered as 'active' netdev.
>>
>> This patch is based on the discussion initiated by Jesse on this thread.
>> https://marc.info/?l=linux-virtualization&m=151189725224231&w=2
>>
>> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>
>> +static void
>> +virtnet_bypass_get_stats(struct net_device *dev,
>> + struct rtnl_link_stats64 *stats)
>> +{
>> + struct virtnet_bypass_info *vbi = netdev_priv(dev);
>> + const struct rtnl_link_stats64 *new;
>> + struct rtnl_link_stats64 temp;
>> + struct net_device *child_netdev;
>> +
>> + spin_lock(&vbi->stats_lock);
>> + memcpy(stats, &vbi->bypass_stats, sizeof(*stats));
>> +
>> + rcu_read_lock();
>> +
>> + child_netdev = rcu_dereference(vbi->active_netdev);
>> + if (child_netdev) {
>> + new = dev_get_stats(child_netdev, &temp);
>> + virtnet_bypass_fold_stats(stats, new, &vbi->active_stats);
>> + memcpy(&vbi->active_stats, new, sizeof(*new));
>> + }
>> +
>> + child_netdev = rcu_dereference(vbi->backup_netdev);
>> + if (child_netdev) {
>> + new = dev_get_stats(child_netdev, &temp);
>> + virtnet_bypass_fold_stats(stats, new, &vbi->backup_stats);
>> + memcpy(&vbi->backup_stats, new, sizeof(*new));
>> + }
>> +
>> + rcu_read_unlock();
>> +
>> + memcpy(&vbi->bypass_stats, stats, sizeof(*stats));
>> + spin_unlock(&vbi->stats_lock);
>> +}
>> +
>> +static int virtnet_bypass_change_mtu(struct net_device *dev, int new_mtu)
>> +{
>> + struct virtnet_bypass_info *vbi = netdev_priv(dev);
>> + struct net_device *child_netdev;
>> + int ret = 0;
>> +
>> + child_netdev = rcu_dereference(vbi->active_netdev);
>> + if (child_netdev) {
>> + ret = dev_set_mtu(child_netdev, new_mtu);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + child_netdev = rcu_dereference(vbi->backup_netdev);
>> + if (child_netdev) {
>> + ret = dev_set_mtu(child_netdev, new_mtu);
>> + if (ret)
>> + netdev_err(child_netdev,
>> + "Unexpected failure to set mtu to %d\n",
>> + new_mtu);
>
> You should probably unwind if set fails on one of the legs.
Actually if we know that the backup is always going to be a virtio I
don't know if we even need to worry about the call failing. Last I
knew virtio_net doesn't implement ndo_change_mtu so I don't think it
is an issue. Unless a notifier blows up about it somewhere I don't
think there is anything that should prevent us from updating the MTU.
One interesting thing we may want to take a look at would be to tweak
the ordering of things based on if we are increasing or decreasing the
MTU. In the case of a increase we need to work from the bottom up, but
in the case of a decrease I wonder if we shouldn't be working from the
top down in order to guarantee we don't create an MTU mismatch
somewhere in the path.
>> + }
>> +
>> + dev->mtu = new_mtu;
>> + return 0;
>> +}
>
> nit: stats, mtu, all those mundane things are implemented in team
> already. If we had this as kernel-internal team mode we wouldn't
> have to reimplement them... You probably did investigate that
> option, for my edification, would you mind saying what the
> challenges/downsides were?
So I tried working with the bonding driver to get down to what we
needed. The issue is there are so many controls and such exposed that
trying to pull them out and generate a simple bond became very
difficult to get done. It was just much easier to start over versus
trying to take an existing interface and pare it down to just what we
needed. What may make more sense is to in the future create either a
lib or some sort of file in net/core/ that we could consolidate the
core functionality of these type of devices into and leave the
user-space interfaces, debugfs, ioctls, and such out of and leave
those driver specific.
>> +static struct net_device *
>> +get_virtnet_bypass_bymac(struct net *net, const u8 *mac)
>> +{
>> + struct net_device *dev;
>> +
>> + ASSERT_RTNL();
>> +
>> + for_each_netdev(net, dev) {
>> + if (dev->netdev_ops != &virtnet_bypass_netdev_ops)
>> + continue; /* not a virtnet_bypass device */
>
> Is there anything inherently wrong with enslaving another virtio dev
> now? I was expecting something like a hash map to map MAC addr ->
> master and then one can check if dev is already enslaved to that master.
> Just a random thought, I'm probably missing something...
This isn't about enslaving, this is just finding the master device.
Basically the virtnet_bypass uses a separate set of netdev ops so we
just are using that instead of maintaining a global or per-net hash.
You could have two virtio devices enslaved by the same virtnet_bypass
and it shouldn't be an issue.
>> + if (ether_addr_equal(mac, dev->perm_addr))
>> + return dev;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +static struct net_device *
>> +get_virtnet_bypass_byref(struct net_device *child_netdev)
>> +{
>> + struct net *net = dev_net(child_netdev);
>> + struct net_device *dev;
>> +
>> + ASSERT_RTNL();
>> +
>> + for_each_netdev(net, dev) {
>> + struct virtnet_bypass_info *vbi;
>> +
>> + if (dev->netdev_ops != &virtnet_bypass_netdev_ops)
>> + continue; /* not a virtnet_bypass device */
>> +
>> + vbi = netdev_priv(dev);
>> +
>> + if ((rtnl_dereference(vbi->active_netdev) == child_netdev) ||
>> + (rtnl_dereference(vbi->backup_netdev) == child_netdev))
>
> nit: parens not needed
Yeah, it is a habit of mine since I do that for readability more than
anything else. Some people can't track the order of operations.
If they need to go they can.
>> + return dev; /* a match */
>> + }
>> +
>> + return NULL;
>> +}
>
>> +static int virtnet_bypass_create(struct virtnet_info *vi)
>> +{
>> + struct net_device *backup_netdev = vi->dev;
>> + struct device *dev = &vi->vdev->dev;
>> + struct net_device *bypass_netdev;
>> + int res;
>> +
>> + /* Alloc at least 2 queues, for now we are going with 16 assuming
>> + * that most devices being bonded won't have too many queues.
>> + */
>> + bypass_netdev = alloc_etherdev_mq(sizeof(struct virtnet_bypass_info),
>> + 16);
>> + if (!bypass_netdev) {
>> + dev_err(dev, "Unable to allocate bypass_netdev!\n");
>> + return -ENOMEM;
>> + }
>
> Maybe it's just me but referring to master as bypass seems slightly
> confusing. I know you don't like team and bond, but perhaps we can
> come up with a better name? For me bypass device is the other leg,
> i.e. the VF, not the master. Perhaps others disagree.
The choice of naming is based on some basic plumbing ideas. You could
almost think of the bypass netdev as more of a bypass valve. Basically
what we are doing is rerouting the traffic at the bypass interface and
sending it to the VF instead of routing it to the virtio_net. That was
why I thought it would be a fitting term. It gets us out of the
"bond", "team", and other such concepts because it isn't really any of
those. This is supposed to be a very simple device that will shunt the
traffic off of the virtio_net and re-route it through the VF. In
addition this isn't a complete solution by itself either since there
will still be some configuration required on the host to take care of
applying filters on the PF and/or tap interface to make certain we
don't end up with any loops in the traffic and that receives are
directed to the VF.
>> + dev_net_set(bypass_netdev, dev_net(backup_netdev));
>> + SET_NETDEV_DEV(bypass_netdev, dev);
>> +
>> + bypass_netdev->netdev_ops = &virtnet_bypass_netdev_ops;
>> + bypass_netdev->ethtool_ops = &virtnet_bypass_ethtool_ops;
>
> Thanks!
^ permalink raw reply
* Re: [RFC PATCH v3 0/3] Enable virtio_net to act as a backup for a passthru device
From: Alexander Duyck @ 2018-02-17 17:12 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Duyck, Alexander H, virtio-dev, Michael S. Tsirkin,
Sridhar Samudrala, virtualization, Siwei Liu, Netdev,
David Miller
In-Reply-To: <20180216183817.42b07af6@cakuba.netronome.com>
On Fri, Feb 16, 2018 at 6:38 PM, Jakub Kicinski <kubakici@wp.pl> wrote:
> On Fri, 16 Feb 2018 10:11:19 -0800, Sridhar Samudrala wrote:
>> Ppatch 2 is in response to the community request for a 3 netdev
>> solution. However, it creates some issues we'll get into in a moment.
>> It extends virtio_net to use alternate datapath when available and
>> registered. When BACKUP feature is enabled, virtio_net driver creates
>> an additional 'bypass' netdev that acts as a master device and controls
>> 2 slave devices. The original virtio_net netdev is registered as
>> 'backup' netdev and a passthru/vf device with the same MAC gets
>> registered as 'active' netdev. Both 'bypass' and 'backup' netdevs are
>> associated with the same 'pci' device. The user accesses the network
>> interface via 'bypass' netdev. The 'bypass' netdev chooses 'active' netdev
>> as default for transmits when it is available with link up and running.
>
> Thank you do doing this.
>
>> We noticed a couple of issues with this approach during testing.
>> - As both 'bypass' and 'backup' netdevs are associated with the same
>> virtio pci device, udev tries to rename both of them with the same name
>> and the 2nd rename will fail. This would be OK as long as the first netdev
>> to be renamed is the 'bypass' netdev, but the order in which udev gets
>> to rename the 2 netdevs is not reliable.
>
> Out of curiosity - why do you link the master netdev to the virtio
> struct device?
The basic idea of all this is that we wanted this to work with an
existing VM image that was using virtio. As such we were trying to
make it so that the bypass interface takes the place of the original
virtio and get udev to rename the bypass to what the original
virtio_net was.
> FWIW two solutions that immediately come to mind is to export "backup"
> as phys_port_name of the backup virtio link and/or assign a name to the
> master like you are doing already. I think team uses team%d and bond
> uses bond%d, soft naming of master devices seems quite natural in this
> case.
I figured I had overlooked something like that.. Thanks for pointing
this out. Okay so I think the phys_port_name approach might resolve
the original issue. If I am reading things correctly what we end up
with is the master showing up as "ens1" for example and the backup
showing up as "ens1nbackup". Am I understanding that right?
The problem with the team/bond%d approach is that it creates a new
netdevice and so it would require guest configuration changes.
> IMHO phys_port_name == "backup" if BACKUP bit is set on slave virtio
> link is quite neat.
I agree. For non-"backup" virio_net devices would it be okay for us to
just return -EOPNOTSUPP? I assume it would be and that way the legacy
behavior could be maintained although the function still exists.
>> - When the 'active' netdev is unplugged OR not present on a destination
>> system after live migration, the user will see 2 virtio_net netdevs.
>
> That's necessary and expected, all configuration applies to the master
> so master must exist.
With the naming issue resolved this is the only item left outstanding.
This becomes a matter of form vs function.
The main complaint about the "3 netdev" solution is a bit confusing to
have the 2 netdevs present if the VF isn't there. The idea is that
having the extra "master" netdev there if there isn't really a bond is
a bit ugly.
The downside of the "2 netdev" solution is that you have to deal with
an extra layer of locking/queueing to get to the VF and you lose some
functionality since things like in-driver XDP have to be disabled in
order to maintain the same functionality when the VF is present or
not. However it looks more like classic virtio_net when the VF is not
present.
^ permalink raw reply
* (unknown),
From: Solen win @ 2018-02-17 8:41 UTC (permalink / raw)
To: Virtualization
[-- Attachment #1.1: Type: text/plain, Size: 8 bytes --]
Confirm
[-- Attachment #1.2: Type: text/html, Size: 30 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH] virtio_balloon: export huge page allocation statistics
From: Jonathan Helman @ 2018-02-17 5:44 UTC (permalink / raw)
To: mst, jasowang, virtualization; +Cc: linux-kernel
Export statistics for successful and failed huge page allocations
from the virtio balloon driver. These 2 stats come directly from
the vm_events HTLB_BUDDY_PGALLOC and HTLB_BUDDY_PGALLOC_FAIL.
Signed-off-by: Jonathan Helman <jonathan.helman@oracle.com>
---
drivers/virtio/virtio_balloon.c | 6 ++++++
include/uapi/linux/virtio_balloon.h | 4 +++-
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index dfe5684..6b237e3 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -272,6 +272,12 @@ static unsigned int update_balloon_stats(struct virtio_balloon *vb)
pages_to_bytes(events[PSWPOUT]));
update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
+#ifdef CONFIG_HUGETLB_PAGE
+ update_stat(vb, idx++, VIRTIO_BALLOON_S_HTLB_PGALLOC,
+ events[HTLB_BUDDY_PGALLOC]);
+ update_stat(vb, idx++, VIRTIO_BALLOON_S_HTLB_PGFAIL,
+ events[HTLB_BUDDY_PGALLOC_FAIL]);
+#endif
#endif
update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
pages_to_bytes(i.freeram));
diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
index 4e8b830..e3e8071 100644
--- a/include/uapi/linux/virtio_balloon.h
+++ b/include/uapi/linux/virtio_balloon.h
@@ -53,7 +53,9 @@ struct virtio_balloon_config {
#define VIRTIO_BALLOON_S_MEMTOT 5 /* Total amount of memory */
#define VIRTIO_BALLOON_S_AVAIL 6 /* Available memory as in /proc */
#define VIRTIO_BALLOON_S_CACHES 7 /* Disk caches */
-#define VIRTIO_BALLOON_S_NR 8
+#define VIRTIO_BALLOON_S_HTLB_PGALLOC 8 /* Number of htlb pgalloc successes */
+#define VIRTIO_BALLOON_S_HTLB_PGFAIL 9 /* Number of htlb pgalloc failures */
+#define VIRTIO_BALLOON_S_NR 10
/*
* Memory statistics structure.
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH v2 0/6] crypto: engine - Permit to enqueue all async requests
From: Herbert Xu @ 2018-02-17 4:42 UTC (permalink / raw)
To: Corentin Labbe
Cc: alexandre.torgue, corbet, mst, linux-doc, linux-kernel,
fabien.dessenne, virtualization, linux-sunxi, linux-crypto,
mcoquelin.stm32, davem, linux-arm-kernel
In-Reply-To: <20180216153656.GA22492@Red>
On Fri, Feb 16, 2018 at 04:36:56PM +0100, Corentin Labbe wrote:
>
> As mentionned in the cover letter, all patchs (except documentation one) should be squashed.
> A kbuild robot reported a build error on cryptodev due to this.
It's too late now. In future if you want the patches to be squashed
then please send them in one email.
Thanks,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [RFC PATCH v3 2/3] virtio_net: Extend virtio to use VF datapath when available
From: Jakub Kicinski @ 2018-02-17 3:04 UTC (permalink / raw)
To: Sridhar Samudrala
Cc: alexander.h.duyck, Or Gerlitz, mst, netdev, virtualization,
loseweigh, davem
In-Reply-To: <1518804682-16881-3-git-send-email-sridhar.samudrala@intel.com>
On Fri, 16 Feb 2018 10:11:21 -0800, Sridhar Samudrala wrote:
> This patch enables virtio_net to switch over to a VF datapath when a VF
> netdev is present with the same MAC address. It allows live migration
> of a VM with a direct attached VF without the need to setup a bond/team
> between a VF and virtio net device in the guest.
>
> The hypervisor needs to enable only one datapath at any time so that
> packets don't get looped back to the VM over the other datapath. When a VF
> is plugged, the virtio datapath link state can be marked as down. The
> hypervisor needs to unplug the VF device from the guest on the source host
> and reset the MAC filter of the VF to initiate failover of datapath to
> virtio before starting the migration. After the migration is completed,
> the destination hypervisor sets the MAC filter on the VF and plugs it back
> to the guest to switch over to VF datapath.
>
> When BACKUP feature is enabled, an additional netdev(bypass netdev) is
> created that acts as a master device and tracks the state of the 2 lower
> netdevs. The original virtio_net netdev is marked as 'backup' netdev and a
> passthru device with the same MAC is registered as 'active' netdev.
>
> This patch is based on the discussion initiated by Jesse on this thread.
> https://marc.info/?l=linux-virtualization&m=151189725224231&w=2
>
> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> +static void
> +virtnet_bypass_get_stats(struct net_device *dev,
> + struct rtnl_link_stats64 *stats)
> +{
> + struct virtnet_bypass_info *vbi = netdev_priv(dev);
> + const struct rtnl_link_stats64 *new;
> + struct rtnl_link_stats64 temp;
> + struct net_device *child_netdev;
> +
> + spin_lock(&vbi->stats_lock);
> + memcpy(stats, &vbi->bypass_stats, sizeof(*stats));
> +
> + rcu_read_lock();
> +
> + child_netdev = rcu_dereference(vbi->active_netdev);
> + if (child_netdev) {
> + new = dev_get_stats(child_netdev, &temp);
> + virtnet_bypass_fold_stats(stats, new, &vbi->active_stats);
> + memcpy(&vbi->active_stats, new, sizeof(*new));
> + }
> +
> + child_netdev = rcu_dereference(vbi->backup_netdev);
> + if (child_netdev) {
> + new = dev_get_stats(child_netdev, &temp);
> + virtnet_bypass_fold_stats(stats, new, &vbi->backup_stats);
> + memcpy(&vbi->backup_stats, new, sizeof(*new));
> + }
> +
> + rcu_read_unlock();
> +
> + memcpy(&vbi->bypass_stats, stats, sizeof(*stats));
> + spin_unlock(&vbi->stats_lock);
> +}
> +
> +static int virtnet_bypass_change_mtu(struct net_device *dev, int new_mtu)
> +{
> + struct virtnet_bypass_info *vbi = netdev_priv(dev);
> + struct net_device *child_netdev;
> + int ret = 0;
> +
> + child_netdev = rcu_dereference(vbi->active_netdev);
> + if (child_netdev) {
> + ret = dev_set_mtu(child_netdev, new_mtu);
> + if (ret)
> + return ret;
> + }
> +
> + child_netdev = rcu_dereference(vbi->backup_netdev);
> + if (child_netdev) {
> + ret = dev_set_mtu(child_netdev, new_mtu);
> + if (ret)
> + netdev_err(child_netdev,
> + "Unexpected failure to set mtu to %d\n",
> + new_mtu);
You should probably unwind if set fails on one of the legs.
> + }
> +
> + dev->mtu = new_mtu;
> + return 0;
> +}
nit: stats, mtu, all those mundane things are implemented in team
already. If we had this as kernel-internal team mode we wouldn't
have to reimplement them... You probably did investigate that
option, for my edification, would you mind saying what the
challenges/downsides were?
> +static struct net_device *
> +get_virtnet_bypass_bymac(struct net *net, const u8 *mac)
> +{
> + struct net_device *dev;
> +
> + ASSERT_RTNL();
> +
> + for_each_netdev(net, dev) {
> + if (dev->netdev_ops != &virtnet_bypass_netdev_ops)
> + continue; /* not a virtnet_bypass device */
Is there anything inherently wrong with enslaving another virtio dev
now? I was expecting something like a hash map to map MAC addr ->
master and then one can check if dev is already enslaved to that master.
Just a random thought, I'm probably missing something...
> + if (ether_addr_equal(mac, dev->perm_addr))
> + return dev;
> + }
> +
> + return NULL;
> +}
> +
> +static struct net_device *
> +get_virtnet_bypass_byref(struct net_device *child_netdev)
> +{
> + struct net *net = dev_net(child_netdev);
> + struct net_device *dev;
> +
> + ASSERT_RTNL();
> +
> + for_each_netdev(net, dev) {
> + struct virtnet_bypass_info *vbi;
> +
> + if (dev->netdev_ops != &virtnet_bypass_netdev_ops)
> + continue; /* not a virtnet_bypass device */
> +
> + vbi = netdev_priv(dev);
> +
> + if ((rtnl_dereference(vbi->active_netdev) == child_netdev) ||
> + (rtnl_dereference(vbi->backup_netdev) == child_netdev))
nit: parens not needed
> + return dev; /* a match */
> + }
> +
> + return NULL;
> +}
> +static int virtnet_bypass_create(struct virtnet_info *vi)
> +{
> + struct net_device *backup_netdev = vi->dev;
> + struct device *dev = &vi->vdev->dev;
> + struct net_device *bypass_netdev;
> + int res;
> +
> + /* Alloc at least 2 queues, for now we are going with 16 assuming
> + * that most devices being bonded won't have too many queues.
> + */
> + bypass_netdev = alloc_etherdev_mq(sizeof(struct virtnet_bypass_info),
> + 16);
> + if (!bypass_netdev) {
> + dev_err(dev, "Unable to allocate bypass_netdev!\n");
> + return -ENOMEM;
> + }
Maybe it's just me but referring to master as bypass seems slightly
confusing. I know you don't like team and bond, but perhaps we can
come up with a better name? For me bypass device is the other leg,
i.e. the VF, not the master. Perhaps others disagree.
> + dev_net_set(bypass_netdev, dev_net(backup_netdev));
> + SET_NETDEV_DEV(bypass_netdev, dev);
> +
> + bypass_netdev->netdev_ops = &virtnet_bypass_netdev_ops;
> + bypass_netdev->ethtool_ops = &virtnet_bypass_ethtool_ops;
Thanks!
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox