thr3ads.net - Virtualization - [RFC PATCH v3 0/3] Enable virtio_net to act as a backup for a passthru device [Feb 2018]

If this information is useful, please help other people find it:
Share via:

Alexander Duyck

2018-Feb-21 15:56 UTC

[RFC PATCH v3 0/3] Enable virtio_net to act as a backup for a passthru device

On Wed, Feb 21, 2018 at 1:51 AM, Jiri Pirko <jiri at resnulli.us>
wrote:> Tue, Feb 20, 2018 at 11:33:56PM CET, kubakici at 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 at 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,

Jiri Pirko

2018-Feb-21 16:11 UTC

head link

[RFC PATCH v3 0/3] Enable virtio_net to act as a backup for a passthru device

Wed, Feb 21, 2018 at 04:56:48PM CET, alexander.duyck at gmail.com
wrote:>On Wed, Feb 21, 2018 at 1:51 AM, Jiri Pirko <jiri at resnulli.us>
wrote:
>> Tue, Feb 20, 2018 at 11:33:56PM CET, kubakici at 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 at 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,

Alexander Duyck

2018-Feb-21 16:49 UTC

head link

[RFC PATCH v3 0/3] Enable virtio_net to act as a backup for a passthru device

On Wed, Feb 21, 2018 at 8:11 AM, Jiri Pirko <jiri at resnulli.us>
wrote:> Wed, Feb 21, 2018 at 04:56:48PM CET, alexander.duyck at gmail.com wrote:
>>On Wed, Feb 21, 2018 at 1:51 AM, Jiri Pirko <jiri at resnulli.us>
wrote:
>>> Tue, Feb 20, 2018 at 11:33:56PM CET, kubakici at 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 at 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.

Reasonably Related Threads

Search for more apparently analagous threads

Virtualization - Feb 2018 - [RFC PATCH v3 0/3] Enable virtio_net to act as a backup for a passthru device

[RFC PATCH v3 0/3] Enable virtio_net to act as a backup for a passthru device

[RFC PATCH v3 0/3] Enable virtio_net to act as a backup for a passthru device

[RFC PATCH v3 0/3] Enable virtio_net to act as a backup for a passthru device

Reasonably Related Threads