[PATCH] net-af_xdp: use correct number of channels from ethtool

[PATCH] net-af_xdp: use correct number of channels from ethtool

[Luigi Rizzo]

Drivers use different fields to report the number of channels, so take
the maximum of all fields (rx, tx, other, combined) when determining the
size of the xsk map. The current code used only 'combined' which was set
to 0 in some drivers e.g. mlx4.

Tested: compiled and run xdpsock -q 3 -r -S on mlx4
Signed-off-by: Luigi Rizzo <lrizzo@google.com>
---
 tools/lib/bpf/xsk.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
index 74d84f36a5b24..8e12269428d08 100644
--- a/tools/lib/bpf/xsk.c
+++ b/tools/lib/bpf/xsk.c
@@ -412,6 +412,11 @@ static int xsk_load_xdp_prog(struct xsk_socket *xsk)
 	return 0;
 }
 
+static inline int max_i(int a, int b)
+{
+	return a > b ? a : b;
+}
+
 static int xsk_get_max_queues(struct xsk_socket *xsk)
 {
 	struct ethtool_channels channels = { .cmd = ETHTOOL_GCHANNELS };
@@ -431,13 +436,18 @@ static int xsk_get_max_queues(struct xsk_socket *xsk)
 		goto out;
 	}
 
-	if (err || channels.max_combined == 0)
+	if (err) {
 		/* If the device says it has no channels, then all traffic
 		 * is sent to a single stream, so max queues = 1.
 		 */
 		ret = 1;
-	else
-		ret = channels.max_combined;
+	} else {
+		/* Take the max of rx, tx, other, combined. Drivers return
+		 * the number of channels in different ways.
+		 */
+		ret = max_i(max_i(channels.max_rx, channels.max_tx),
+			      max_i(channels.max_other, channels.max_combined));
+	}
 
 out:
 	close(fd);
-- 
2.24.0.432.g9d3f5f5b63-goog

Re: [PATCH] net-af_xdp: use correct number of channels from ethtool

[Jakub Kicinski]

On Mon, 18 Nov 2019 14:55:23 -0800, Luigi Rizzo wrote:
> Drivers use different fields to report the number of channels, so take
> the maximum of all fields (rx, tx, other, combined) when determining the
> size of the xsk map. The current code used only 'combined' which was set
> to 0 in some drivers e.g. mlx4.
> 
> Tested: compiled and run xdpsock -q 3 -r -S on mlx4
> Signed-off-by: Luigi Rizzo <lrizzo@google.com>

thanks, this seems mostly correct

> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> index 74d84f36a5b24..8e12269428d08 100644
> --- a/tools/lib/bpf/xsk.c
> +++ b/tools/lib/bpf/xsk.c
> @@ -412,6 +412,11 @@ static int xsk_load_xdp_prog(struct xsk_socket *xsk)
>  	return 0;
>  }
>  
> +static inline int max_i(int a, int b)
> +{
> +	return a > b ? a : b;
> +}

There's already a max in tools/lib/bpf/libbpf_internal.h, could you
possible just use that?

>  static int xsk_get_max_queues(struct xsk_socket *xsk)
>  {
>  	struct ethtool_channels channels = { .cmd = ETHTOOL_GCHANNELS };
> @@ -431,13 +436,18 @@ static int xsk_get_max_queues(struct xsk_socket *xsk)
>  		goto out;
>  	}
>  
> -	if (err || channels.max_combined == 0)
> +	if (err) {
>  		/* If the device says it has no channels, then all traffic
>  		 * is sent to a single stream, so max queues = 1.
>  		 */
>  		ret = 1;
> -	else
> -		ret = channels.max_combined;
> +	} else {
> +		/* Take the max of rx, tx, other, combined. Drivers return
> +		 * the number of channels in different ways.
> +		 */
> +		ret = max_i(max_i(channels.max_rx, channels.max_tx),
> +			      max_i(channels.max_other, channels.max_combined));

The continuation line should be aligned to the opening bracket.

I don't think we need to care about other, other is for non-traffic
interrupts.

> +	}
>  
>  out:
>  	close(fd);

Re: [PATCH] net-af_xdp: use correct number of channels from ethtool

[Luigi Rizzo]

On Mon, Nov 18, 2019 at 3:10 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Mon, 18 Nov 2019 14:55:23 -0800, Luigi Rizzo wrote:
> > Drivers use different fields to report the number of channels, so take
> > the maximum of all fields (rx, tx, other, combined) when determining the
> > size of the xsk map. The current code used only 'combined' which was set
> > to 0 in some drivers e.g. mlx4.
> >
> > Tested: compiled and run xdpsock -q 3 -r -S on mlx4
> > Signed-off-by: Luigi Rizzo <lrizzo@google.com>
>
> thanks, this seems mostly correct
>
> > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> > index 74d84f36a5b24..8e12269428d08 100644
> > --- a/tools/lib/bpf/xsk.c
> > +++ b/tools/lib/bpf/xsk.c
> > @@ -412,6 +412,11 @@ static int xsk_load_xdp_prog(struct xsk_socket *xsk)
> >       return 0;
> >  }
> >
> > +static inline int max_i(int a, int b)
> > +{
> > +     return a > b ? a : b;
> > +}
>
> There's already a max in tools/lib/bpf/libbpf_internal.h, could you
> possible just use that?

Sure, will send an updated patch. Note that the compiler is actually
picking the max()
macro from tools/include/linux/kernel,h which does not lend to  nesting due to
shadowing (it also requires a cast due to stricter type checking):

make: Entering directory 'upstream/tools/lib/bpf'
  CC       staticobjs/xsk.o
In file included from upstream/tools/include/uapi/linux/ethtool.h:17,
                 from xsk.c:18:
xsk.c: In function ‘xsk_get_max_queues’:
upstream/tools/include/linux/kernel.h:43:12: error: declaration of
‘_max1’ shadows a previous local [-Werror=shadow]
  typeof(x) _max1 = (x);   \
            ^~~~~
xsk.c:443:9: note: in expansion of macro ‘max’
   ret = max(max(channels.max_rx, channels.max_tx),

cheers
luigi