[ovs-dev] [PATCH] netdev_afxdp: Detects combined channels and aborts wrong config

Yi-Hung Wei yihung.wei at gmail.com
Mon Jan 13 19:39:50 UTC 2020


On Thu, Jan 9, 2020 at 5:39 AM Ilya Maximets <i.maximets at ovn.org> wrote:
> On 06.01.2020 22:48, William Tu wrote:
> > On Mon, Dec 23, 2019 at 11:34 AM Yi-Hung Wei <yihung.wei at gmail.com> wrote:
> >>
> >> This patches detects the number of combined channels on a AF_XDP port
> >> via using ethtool interface.  If the number of combined channels
> >> is different from the n_rxq, netdev_afxdp will not be able to get
> >> all the packets from that port.  Thus, abort the port setup when
> >> the case is detected.
> >>
> >> Signed-off-by: Yi-Hung Wei <yihung.wei at gmail.com>
> >
> > Looks good to me, thanks!
> > CC Ilya to see if he has more insight/comments.
> >
> > Acked-by: William Tu <u9012063 at gmail.com>
> >
> >> ---
> >> Travis CI: https://travis-ci.org/YiHungWei/ovs/builds/627972465
> >>
> >> ---
> >>  lib/netdev-afxdp.c | 15 +++++++++++++++
> >>  lib/netdev-linux.c | 27 +++++++++++++++++++++++++++
> >>  lib/netdev-linux.h |  2 ++
> >>  3 files changed, 44 insertions(+)
> >>
> >> diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
> >> index 58365ed483e3..6d002882d355 100644
> >> --- a/lib/netdev-afxdp.c
> >> +++ b/lib/netdev-afxdp.c
> >> @@ -601,6 +601,14 @@ netdev_afxdp_set_config(struct netdev *netdev, const struct smap *args,
> >>      enum afxdp_mode xdp_mode;
> >>      bool need_wakeup;
> >>      int new_n_rxq;
> >> +    int combined_channels;
> >> +
> >> +    if (netdev_linux_ethtool_get_combined_channels(netdev,
> >> +                                                   &combined_channels)) {
> >> +        VLOG_INFO("Cannot get the number of combined channels of %s. "
> >> +                  "Defaults it to 1.", netdev_get_name(netdev));
> >> +        combined_channels = 1;
> >> +    }
> >>
> >>      ovs_mutex_lock(&dev->mutex);
> >>      new_n_rxq = MAX(smap_get_int(args, "n_rxq", NR_QUEUE), 1);
> >> @@ -611,6 +619,13 @@ netdev_afxdp_set_config(struct netdev *netdev, const struct smap *args,
> >>          return EINVAL;
> >>      }
> >>
> >> +    if (new_n_rxq != combined_channels) {
> >> +        ovs_mutex_unlock(&dev->mutex);
> >> +        VLOG_ERR("%s: n_rxq: %d != combined channels: %d.",
> >> +                 netdev_get_name(netdev), new_n_rxq, combined_channels);
> >> +        return EINVAL;
> >> +    }
> >> +
> >>      str_xdp_mode = smap_get_def(args, "xdp-mode", "best-effort");
> >>      for (xdp_mode = OVS_AF_XDP_MODE_BEST_EFFORT;
> >>           xdp_mode < OVS_AF_XDP_MODE_MAX;
> >> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> >> index f8e59bacfb13..e3086fc1cbb6 100644
> >> --- a/lib/netdev-linux.c
> >> +++ b/lib/netdev-linux.c
> >> @@ -2166,6 +2166,33 @@ out:
> >>      netdev->get_features_error = error;
> >>  }
> >>
> >> +int
> >> +netdev_linux_ethtool_get_combined_channels(struct netdev *netdev_,
> >> +                                           int *combined_channels)
> >> +{
> >> +    struct netdev_linux *netdev = netdev_linux_cast(netdev_);
> >> +    struct ethtool_channels echannels;
> >> +    int err;
> >> +
> >> +    ovs_mutex_lock(&netdev->mutex);
> >> +
> >> +    COVERAGE_INC(netdev_get_ethtool);
> >> +
> >> +    memset(&echannels, 0, sizeof echannels);
> >> +    err = netdev_linux_do_ethtool(netdev_get_name(netdev_),
> >> +                                  (struct ethtool_cmd *) &echannels,
> >> +                                  ETHTOOL_GCHANNELS, "ETHTOOL_GCHANNELS");
>
> Does this command return maximum possible number of channels or
> number of currently enabled channels?
Thanks for review.  This command can return both the maximum possible
number of channels and the currently enabled channels.


> In first case we'll end up with a need to configure huge number of queues.
> In second case it's not guaranteed that number of channels will not be
> changed later.  (Does enabling more channels generates if-notifier event?)
Yes, it generates if-notifier event.


> Another point is why we need a configurable parameter that is allowed to
> be configured with a single value only?
>
> And there is a possible usecase for not having all the queues attached
> to OVS.  For example, if you have a custom xdp program loaded, you may
> redirect specific traffic to fast afxdp queues and other traffic to
> kernel or system datapath.  This might be useful for quick handling
> of undesired or malicious traffic.
>
> However, this still make sense to limit the maximum number of queues
> with the number of actually available channels instead of having a
> hardcoded constant (MAX_XSKQ).

I agree that it is a valid use case to attach some queues to OVS and
attach other XDP programs to the other queues.  In this case, it is
not appropriate to fail the afxdp setup when n_rxq != # or combined
channels or # of rx channels.

On the other hand, does it make sense to log the current # of combined
channels and the current # of rx channels in the INFO level?  It could
be helpful to detect configuration issue on both use cases (attach all
all the queues to OVS or not).

I  also agree that it make sense to use the number of maximum
available channels from ethtool rather than MAX_XSKQ to check the
queue configuration. I will include that in the next version.

Thanks,

-Yi-Hung


More information about the dev mailing list