diff mbox series

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

Message ID 20191223193357.94930-1-yihung.wei@gmail.com
State Deferred
Headers show
Series [ovs-dev] netdev_afxdp: Detects combined channels and aborts wrong config | expand

Commit Message

Yi-Hung Wei Dec. 23, 2019, 7:33 p.m. UTC
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@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(+)

Comments

William Tu Jan. 6, 2020, 9:48 p.m. UTC | #1
On Mon, Dec 23, 2019 at 11:34 AM Yi-Hung Wei <yihung.wei@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@gmail.com>

Looks good to me, thanks!
CC Ilya to see if he has more insight/comments.

Acked-by: William Tu <u9012063@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");
> +    if (err) {
> +        goto exit;
> +    }
> +
> +    *combined_channels = echannels.combined_count;
> +
> +exit:
> +    ovs_mutex_unlock(&netdev->mutex);
> +    return err;
> +}
> +
>  /* Stores the features supported by 'netdev' into of '*current', '*advertised',
>   * '*supported', and '*peer'.  Each value is a bitmap of NETDEV_* bits.
>   * Returns 0 if successful, otherwise a positive errno value. */
> diff --git a/lib/netdev-linux.h b/lib/netdev-linux.h
> index e1e30f806557..55cade4bb356 100644
> --- a/lib/netdev-linux.h
> +++ b/lib/netdev-linux.h
> @@ -27,6 +27,8 @@ struct netdev;
>
>  int netdev_linux_ethtool_set_flag(struct netdev *netdev, uint32_t flag,
>                                    const char *flag_name, bool enable);
> +int netdev_linux_ethtool_get_combined_channels(struct netdev *netdev,
> +                                               int *combined_channels);
>  int linux_get_ifindex(const char *netdev_name);
>
>  #endif /* netdev-linux.h */
> --
> 2.17.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ben Pfaff Jan. 6, 2020, 10:15 p.m. UTC | #2
On Mon, Dec 23, 2019 at 11:33:57AM -0800, Yi-Hung Wei 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@gmail.com>
> ---
> Travis CI: https://travis-ci.org/YiHungWei/ovs/builds/627972465

I don't know what a combined channel is.  Should the documentation talk
about this?
Yi-Hung Wei Jan. 9, 2020, 1:59 a.m. UTC | #3
Hi Ben,

After taking closer look at different driver implementations, I found
that the use of combined channels is actually driver dependent.

From ethtool (8), a channel is an IRQ and the set of queues that can
trigger that IRQ.  I tend to think channel as rx/tx queues from
software's point of view.  There are 4 types of channels that ethtool
can configure, rx, tx, other, and combined.

For Intel NICs, (at least for NICs that use ixgbe, i40 driver), we can
only configure channels through 'combined' parameter. It will set both
rx and tx channel to N with the following command.
$ ethool -L eth0 combined N

However, it does not support configuring rx and tx channel separately.
We can only set/get number of combined channel, but not rx or tx
channel.
For example,
$ ethtool -l eth0
Channel parameters for eth0:
Pre-set maximums:
RX:        0
TX:        0
Other:        1
Combined:    63
Current hardware settings:
RX:        0
TX:        0
Other:        1
Combined:    8

On the other hand, NICs that use Broadcom tg3 drivers can configure
its rx and tx channel individually, but can not set with 'combined'
parameter. For Mellanox, mlx4 driver supports set/get rx/tx channels,
but mlx5 driver can only configured through combined channel.

Another ethtool example with Broadcom NetXtreme BCM5720 NIC.
$ ethtool -L eno1 rx 2 tx 1
$ ethtool -l eno1
Channel parameters for eno1:
Pre-set maximums:
RX:             4
TX:             4
Other:          0
Combined:       0
Current hardware settings:
RX:             2
TX:             1
Other:          0
Combined:       0

Back to this patch, what I was trying to do is to make sure that the
number of rx channels reported from ethtool is equal to n_rxq in
AF_XDP's port setting.  Since, I was using a testbed with Intel NIC, I
assume to get # of rx channels from 'combined'.  To support other
types of NICs, in the next version, I will first compare n_rxq with
'rx'. In case of 'rx' is 0,  'combined' will be used.  I will update
the related documentation as well.

Thanks,

-Yi-Hung


On Mon, Jan 6, 2020 at 2:15 PM Ben Pfaff <blp@ovn.org> wrote:
>
> On Mon, Dec 23, 2019 at 11:33:57AM -0800, Yi-Hung Wei 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@gmail.com>
> > ---
> > Travis CI: https://travis-ci.org/YiHungWei/ovs/builds/627972465
>
> I don't know what a combined channel is.  Should the documentation talk
> about this?
Ilya Maximets Jan. 9, 2020, 1:39 p.m. UTC | #4
On 06.01.2020 22:48, William Tu wrote:
> On Mon, Dec 23, 2019 at 11:34 AM Yi-Hung Wei <yihung.wei@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@gmail.com>
> 
> Looks good to me, thanks!
> CC Ilya to see if he has more insight/comments.
> 
> Acked-by: William Tu <u9012063@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?

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?)

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).

Best regards, Ilya Maximets.
Ben Pfaff Jan. 9, 2020, 6:22 p.m. UTC | #5
OK.  I guess that "combined" is what I would would have called "duplex":
going in both directions.

If there's a need to configure this kind of thing, then I think that the
documentation should summarize the kinds of channels and point to
further documentation.

On Wed, Jan 08, 2020 at 05:59:59PM -0800, Yi-Hung Wei wrote:
> Hi Ben,
> 
> After taking closer look at different driver implementations, I found
> that the use of combined channels is actually driver dependent.
> 
> From ethtool (8), a channel is an IRQ and the set of queues that can
> trigger that IRQ.  I tend to think channel as rx/tx queues from
> software's point of view.  There are 4 types of channels that ethtool
> can configure, rx, tx, other, and combined.
> 
> For Intel NICs, (at least for NICs that use ixgbe, i40 driver), we can
> only configure channels through 'combined' parameter. It will set both
> rx and tx channel to N with the following command.
> $ ethool -L eth0 combined N
> 
> However, it does not support configuring rx and tx channel separately.
> We can only set/get number of combined channel, but not rx or tx
> channel.
> For example,
> $ ethtool -l eth0
> Channel parameters for eth0:
> Pre-set maximums:
> RX:        0
> TX:        0
> Other:        1
> Combined:    63
> Current hardware settings:
> RX:        0
> TX:        0
> Other:        1
> Combined:    8
> 
> On the other hand, NICs that use Broadcom tg3 drivers can configure
> its rx and tx channel individually, but can not set with 'combined'
> parameter. For Mellanox, mlx4 driver supports set/get rx/tx channels,
> but mlx5 driver can only configured through combined channel.
> 
> Another ethtool example with Broadcom NetXtreme BCM5720 NIC.
> $ ethtool -L eno1 rx 2 tx 1
> $ ethtool -l eno1
> Channel parameters for eno1:
> Pre-set maximums:
> RX:             4
> TX:             4
> Other:          0
> Combined:       0
> Current hardware settings:
> RX:             2
> TX:             1
> Other:          0
> Combined:       0
> 
> Back to this patch, what I was trying to do is to make sure that the
> number of rx channels reported from ethtool is equal to n_rxq in
> AF_XDP's port setting.  Since, I was using a testbed with Intel NIC, I
> assume to get # of rx channels from 'combined'.  To support other
> types of NICs, in the next version, I will first compare n_rxq with
> 'rx'. In case of 'rx' is 0,  'combined' will be used.  I will update
> the related documentation as well.
> 
> Thanks,
> 
> -Yi-Hung
> 
> 
> On Mon, Jan 6, 2020 at 2:15 PM Ben Pfaff <blp@ovn.org> wrote:
> >
> > On Mon, Dec 23, 2019 at 11:33:57AM -0800, Yi-Hung Wei 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@gmail.com>
> > > ---
> > > Travis CI: https://travis-ci.org/YiHungWei/ovs/builds/627972465
> >
> > I don't know what a combined channel is.  Should the documentation talk
> > about this?
Yi-Hung Wei Jan. 13, 2020, 7:39 p.m. UTC | #6
On Thu, Jan 9, 2020 at 5:39 AM Ilya Maximets <i.maximets@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@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@gmail.com>
> >
> > Looks good to me, thanks!
> > CC Ilya to see if he has more insight/comments.
> >
> > Acked-by: William Tu <u9012063@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
William Tu Jan. 16, 2020, 7:21 p.m. UTC | #7
Thanks for the feedback.

The original problem we faced is that when using AF_XDP on physical device,
users often forgets to setup combined channel (ethool -L eth0 combined N).
Assume the device's default combined channel is 8, and ovs creates only 1 n_rxq,
(ovs-vsctl -- set int eth0 n_rxq=1 ...). Then there is no warning
message shows up,
and device attached to OVS successfully but receives no packet. Because XDP
program is attached to all 8 queues, and only 1 queues has XDP socket
to receives
packets. So this patch adds some warnings.

> >
> > 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.

I think when combined channel changed, the device goes through some reset
process, ex: to drain the queues and re-assign queue id.
>
>
> > Another point is why we need a configurable parameter that is allowed to
> > be configured with a single value only?
I don't understand.

> >
> > 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.

Yes, that's an interesting use case! This will require loading custom
XDP program,
instead of the built-in one in libbpf.
I will send out new version of the custom XDP program patch.
https://patchwork.ozlabs.org/patch/1199734/

> >
> > 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.
>
I think we should fail, or at least log error message.
When n_rxq != # combined channels, OVS attaches XDP programs to all
combined channels, but only handle n_rxq queues. Packets will be dropped
at the rest queues. Remember right now, the XDP program attachment is
per-netdev to all queues, and there is no per-queue XDP program support.

> 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 would live to at least log the n_rxq and combined channels.

Thanks
William
diff mbox series

Patch

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");
+    if (err) {
+        goto exit;
+    }
+
+    *combined_channels = echannels.combined_count;
+
+exit:
+    ovs_mutex_unlock(&netdev->mutex);
+    return err;
+}
+
 /* Stores the features supported by 'netdev' into of '*current', '*advertised',
  * '*supported', and '*peer'.  Each value is a bitmap of NETDEV_* bits.
  * Returns 0 if successful, otherwise a positive errno value. */
diff --git a/lib/netdev-linux.h b/lib/netdev-linux.h
index e1e30f806557..55cade4bb356 100644
--- a/lib/netdev-linux.h
+++ b/lib/netdev-linux.h
@@ -27,6 +27,8 @@  struct netdev;
 
 int netdev_linux_ethtool_set_flag(struct netdev *netdev, uint32_t flag,
                                   const char *flag_name, bool enable);
+int netdev_linux_ethtool_get_combined_channels(struct netdev *netdev,
+                                               int *combined_channels);
 int linux_get_ifindex(const char *netdev_name);
 
 #endif /* netdev-linux.h */