[RFC PATCH 0/3] veth: more flexible channels number configuration

[RFC PATCH 0/3] veth: more flexible channels number configuration

[Paolo Abeni]

XDP setups can benefit from multiple veth RX/TX queues. Currently
veth allow setting such number only at creation time via the 
'numrxqueues' and 'numtxqueues' parameters.

This series introduces support for the ethtool set_channel operation
and allows configuring the queue number via a new module parameter.

The veth default configuration is not changed.

Finally self-tests are updated to check the new features, with both
valid and invalid arguments.

Paolo Abeni (3):
  veth: implement support for set_channel ethtool op
  veth: make queues nr configurable via kernel module params
  selftests: net: veth: add tests for set_channel

 drivers/net/veth.c                  | 83 +++++++++++++++++++++++++++--
 tools/testing/selftests/net/veth.sh | 65 ++++++++++++++++++++++
 2 files changed, 144 insertions(+), 4 deletions(-)

-- 
2.26.3

[RFC PATCH 1/3] veth: implement support for set_channel ethtool op

[Paolo Abeni]

This change implements the set_channel() ethtool operation,
preserving the current defaults values and allowing up set
the number of queues in the range set ad device creation
time.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 drivers/net/veth.c | 62 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 58 insertions(+), 4 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index bdb7ce3cb054..10360228a06a 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -72,6 +72,8 @@ struct veth_priv {
 	atomic64_t		dropped;
 	struct bpf_prog		*_xdp_prog;
 	struct veth_rq		*rq;
+	unsigned int		num_tx_queues;
+	unsigned int		num_rx_queues;
 	unsigned int		requested_headroom;
 };
 
@@ -224,10 +226,49 @@ static void veth_get_channels(struct net_device *dev,
 {
 	channels->tx_count = dev->real_num_tx_queues;
 	channels->rx_count = dev->real_num_rx_queues;
-	channels->max_tx = dev->real_num_tx_queues;
-	channels->max_rx = dev->real_num_rx_queues;
+	channels->max_tx = dev->num_tx_queues;
+	channels->max_rx = dev->num_rx_queues;
 	channels->combined_count = min(dev->real_num_rx_queues, dev->real_num_tx_queues);
-	channels->max_combined = min(dev->real_num_rx_queues, dev->real_num_tx_queues);
+	channels->max_combined = min(dev->num_rx_queues, dev->num_tx_queues);
+}
+
+static int veth_close(struct net_device *dev);
+static int veth_open(struct net_device *dev);
+
+static int veth_set_channels(struct net_device *dev,
+			     struct ethtool_channels *ch)
+{
+	struct veth_priv *priv = netdev_priv(dev);
+	struct veth_priv *peer_priv;
+
+	/* accept changes only on rx/tx */
+	if (ch->combined_count != min(dev->real_num_rx_queues, dev->real_num_tx_queues))
+		return -EINVAL;
+
+	/* respect contraint posed at device creation time */
+	if (ch->rx_count > dev->num_rx_queues || ch->tx_count > dev->num_tx_queues)
+		return -EINVAL;
+
+	if (!ch->rx_count || !ch->tx_count)
+		return -EINVAL;
+
+	/* avoid braking XDP, if that is enabled */
+	if (priv->_xdp_prog && ch->rx_count < priv->peer->real_num_tx_queues)
+		return -EINVAL;
+
+	peer_priv = netdev_priv(priv->peer);
+	if (peer_priv->_xdp_prog && ch->tx_count > priv->peer->real_num_rx_queues)
+		return -EINVAL;
+
+	if (netif_running(dev))
+		veth_close(dev);
+
+	priv->num_tx_queues = ch->tx_count;
+	priv->num_rx_queues = ch->rx_count;
+
+	if (netif_running(dev))
+		veth_open(dev);
+	return 0;
 }
 
 static const struct ethtool_ops veth_ethtool_ops = {
@@ -239,6 +280,7 @@ static const struct ethtool_ops veth_ethtool_ops = {
 	.get_link_ksettings	= veth_get_link_ksettings,
 	.get_ts_info		= ethtool_op_get_ts_info,
 	.get_channels		= veth_get_channels,
+	.set_channels		= veth_set_channels,
 };
 
 /* general routines */
@@ -1104,6 +1146,14 @@ static int veth_open(struct net_device *dev)
 	if (!peer)
 		return -ENOTCONN;
 
+	err = netif_set_real_num_rx_queues(dev, priv->num_rx_queues);
+	if (err)
+		return err;
+
+	err = netif_set_real_num_tx_queues(dev, priv->num_tx_queues);
+	if (err)
+		return err;
+
 	if (priv->_xdp_prog) {
 		err = veth_enable_xdp(dev);
 		if (err)
@@ -1551,14 +1601,18 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 	netif_carrier_off(dev);
 
 	/*
-	 * tie the deviced together
+	 * tie the deviced together and init the default queue nr
 	 */
 
 	priv = netdev_priv(dev);
 	rcu_assign_pointer(priv->peer, peer);
+	priv->num_tx_queues = dev->num_tx_queues;
+	priv->num_rx_queues = dev->num_rx_queues;
 
 	priv = netdev_priv(peer);
 	rcu_assign_pointer(priv->peer, dev);
+	priv->num_tx_queues = peer->num_tx_queues;
+	priv->num_rx_queues = peer->num_rx_queues;
 
 	veth_disable_gro(dev);
 	return 0;
-- 
2.26.3

[RFC PATCH 2/3] veth: make queues nr configurable via kernel module params

[Paolo Abeni]

This allows configuring the number of tx and rx queues at
module load time. A single module parameter controls
both the default number of RX and TX queues created
at device registration time.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 drivers/net/veth.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 10360228a06a..787b4ad2cc87 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -27,6 +27,11 @@
 #include <linux/bpf_trace.h>
 #include <linux/net_tstamp.h>
 
+static int queues_nr	= 1;
+
+module_param(queues_nr, int, 0644);
+MODULE_PARM_DESC(queues_nr, "Max number of RX and TX queues (default = 1)");
+
 #define DRV_NAME	"veth"
 #define DRV_VERSION	"1.0"
 
@@ -1662,6 +1667,18 @@ static struct net *veth_get_link_net(const struct net_device *dev)
 	return peer ? dev_net(peer) : dev_net(dev);
 }
 
+unsigned int veth_get_num_tx_queues(void)
+{
+	/* enforce the same queue limit as rtnl_create_link */
+	int queues = queues_nr;
+
+	if (queues < 1)
+		queues = 1;
+	if (queues > 4096)
+		queues = 4096;
+	return queues;
+}
+
 static struct rtnl_link_ops veth_link_ops = {
 	.kind		= DRV_NAME,
 	.priv_size	= sizeof(struct veth_priv),
@@ -1672,6 +1689,10 @@ static struct rtnl_link_ops veth_link_ops = {
 	.policy		= veth_policy,
 	.maxtype	= VETH_INFO_MAX,
 	.get_link_net	= veth_get_link_net,
+	.get_num_tx_queues	= veth_get_num_tx_queues,
+	.get_num_rx_queues	= veth_get_num_tx_queues, /* Use the same number
+							   * as for TX queues
+							   */
 };
 
 /*
-- 
2.26.3

[RFC PATCH 3/3] selftests: net: veth: add tests for set_channel

[Paolo Abeni]

Simple functional test for the newly exposted features

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 tools/testing/selftests/net/veth.sh | 65 +++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/tools/testing/selftests/net/veth.sh b/tools/testing/selftests/net/veth.sh
index 11d7cdb898c0..1f4cdbe6ffe0 100755
--- a/tools/testing/selftests/net/veth.sh
+++ b/tools/testing/selftests/net/veth.sh
@@ -75,6 +75,31 @@ chk_tso_flag() {
 	__chk_flag "$1" $2 $3 tcp-segmentation-offload
 }
 
+chk_channels() {
+	local msg="$1"
+	local target=$2
+	local rx=$3
+	local tx=$4
+
+	local dev=veth$target
+	local combined=$tx
+	[ $rx -lt $tx ] && combined=$rx
+
+	local cur_rx=`ip netns exec $BASE$target ethtool -l $dev |\
+		grep RX: | tail -n 1 | awk '{print $2}' `
+	local cur_tx=`ip netns exec $BASE$target ethtool -l $dev |\
+		grep TX: | tail -n 1 | awk '{print $2}'`
+	local cur_combined=`ip netns exec $BASE$target ethtool -l $dev |\
+		grep Combined: | tail -n 1 | awk '{print $2}'`
+
+	printf "%-60s" "$msg"
+	if [ "$cur_rx" = "$rx" -a "$cur_tx" = "$tx" -a "$cur_combined" = "$combined" ]; then
+		echo " ok "
+	else
+		echo " fail rx:$rx:$cur_rx tx:$tx:$cur_tx combined:$combined:$cur_combined"
+	fi
+}
+
 chk_gro() {
 	local msg="$1"
 	local expected=$2
@@ -122,6 +147,8 @@ ip netns exec $NS_SRC ethtool -K veth$SRC tx-udp-segmentation off
 chk_gro "        - aggregation with TSO off" 10
 cleanup
 
+# reset default, just in case
+echo 1 > /sys/module/veth/parameters/tx_queues
 create_ns
 ip netns exec $NS_DST ethtool -K veth$DST gro on
 chk_gro_flag "with gro on - gro flag" $DST on
@@ -134,6 +161,11 @@ chk_gro "        - aggregation with TSO off" 1
 cleanup
 
 create_ns
+chk_channels "default channels" $DST 1 1
+
+# will affect next veth device pair creation
+echo 128 > /sys/module/veth/parameters/tx_queues
+
 ip -n $NS_DST link set dev veth$DST down
 ip netns exec $NS_DST ethtool -K veth$DST gro on
 chk_gro_flag "with gro enabled on link down - gro flag" $DST on
@@ -147,7 +179,35 @@ chk_gro "        - aggregation with TSO off" 1
 cleanup
 
 create_ns
+
+ip netns exec $NS_DST ethtool -L veth$DST tx 2
+chk_channels "setting tx channels" $DST 128 2
+
+ip netns exec $NS_DST ethtool -L veth$DST rx 3 tx 4
+chk_channels "setting both rx and tx channels" $DST 3 4
+ip netns exec $NS_DST ethtool -L veth$DST combined 2 2>/dev/null
+chk_channels "bad setting: combined channels" $DST 3 4
+
+ip netns exec $NS_DST ethtool -L veth$DST tx 1025 2>/dev/null
+chk_channels "setting invalid channels nr" $DST 3 4
+
+printf "%-60s" "bad setting: XDP with RX nr less than TX"
+ip -n $NS_DST link set dev veth$DST xdp object ../bpf/xdp_dummy.o section xdp_dummy 2>/dev/null &&\
+	echo "fail - set operation successful ?!?" || echo " ok "
+
+# the following tests will run with multiple channels active
+ip netns exec $NS_SRC ethtool -L veth$SRC tx 3
+
 ip -n $NS_DST link set dev veth$DST xdp object ../bpf/xdp_dummy.o section xdp_dummy 2>/dev/null
+printf "%-60s" "bad setting: reducing RX nr below peer TX with XDP set"
+ip netns exec $NS_DST ethtool -L veth$DST rx 2 2>/dev/null &&\
+	echo "fail - set operation successful ?!?" || echo " ok "
+printf "%-60s" "bad setting: increasing peer TX nr above RX with XDP set"
+ip netns exec $NS_SRC ethtool -L veth$SRC tx 4 2>/dev/null &&\
+	echo "fail - set operation successful ?!?" || echo " ok "
+
+chk_channels "setting invalid channels nr" $DST 3 4
+
 chk_gro_flag "with xdp attached - gro flag" $DST on
 chk_gro_flag "        - peer gro flag" $SRC off
 chk_tso_flag "        - tso flag" $SRC off
@@ -167,8 +227,13 @@ chk_gro_flag "        - after gro on xdp off, gro flag" $DST on
 chk_gro_flag "        - peer gro flag" $SRC off
 chk_tso_flag "        - tso flag" $SRC on
 chk_tso_flag "        - peer tso flag" $DST on
+
+ip netns exec $NS_DST ethtool -L veth$DST tx 5
+chk_channels "setting tx channels with device down" $DST 3 4
+
 ip -n $NS_DST link set dev veth$DST up
 ip -n $NS_SRC link set dev veth$SRC up
+chk_channels "[takes effect after link up]" $DST 3 5
 chk_gro "        - aggregation" 1
 
 ip netns exec $NS_DST ethtool -K veth$DST gro off
-- 
2.26.3

Re: [RFC PATCH 1/3] veth: implement support for set_channel ethtool op

[Toke Høiland-Jørgensen]

Paolo Abeni <pabeni@redhat.com> writes:

> This change implements the set_channel() ethtool operation,
> preserving the current defaults values and allowing up set
> the number of queues in the range set ad device creation
> time.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  drivers/net/veth.c | 62 +++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 58 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index bdb7ce3cb054..10360228a06a 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -72,6 +72,8 @@ struct veth_priv {
>  	atomic64_t		dropped;
>  	struct bpf_prog		*_xdp_prog;
>  	struct veth_rq		*rq;
> +	unsigned int		num_tx_queues;
> +	unsigned int		num_rx_queues;

Why are these needed to be duplicated? Don't they just duplicate the
'real_num_tx_queues' members in struct net_device?

>  	unsigned int		requested_headroom;
>  };
>  
> @@ -224,10 +226,49 @@ static void veth_get_channels(struct net_device *dev,
>  {
>  	channels->tx_count = dev->real_num_tx_queues;
>  	channels->rx_count = dev->real_num_rx_queues;
> -	channels->max_tx = dev->real_num_tx_queues;
> -	channels->max_rx = dev->real_num_rx_queues;
> +	channels->max_tx = dev->num_tx_queues;
> +	channels->max_rx = dev->num_rx_queues;
>  	channels->combined_count = min(dev->real_num_rx_queues, dev->real_num_tx_queues);
> -	channels->max_combined = min(dev->real_num_rx_queues, dev->real_num_tx_queues);
> +	channels->max_combined = min(dev->num_rx_queues, dev->num_tx_queues);
> +}
> +
> +static int veth_close(struct net_device *dev);
> +static int veth_open(struct net_device *dev);
> +
> +static int veth_set_channels(struct net_device *dev,
> +			     struct ethtool_channels *ch)
> +{
> +	struct veth_priv *priv = netdev_priv(dev);
> +	struct veth_priv *peer_priv;
> +
> +	/* accept changes only on rx/tx */
> +	if (ch->combined_count != min(dev->real_num_rx_queues, dev->real_num_tx_queues))
> +		return -EINVAL;
> +
> +	/* respect contraint posed at device creation time */
> +	if (ch->rx_count > dev->num_rx_queues || ch->tx_count > dev->num_tx_queues)
> +		return -EINVAL;
> +
> +	if (!ch->rx_count || !ch->tx_count)
> +		return -EINVAL;
> +
> +	/* avoid braking XDP, if that is enabled */
> +	if (priv->_xdp_prog && ch->rx_count < priv->peer->real_num_tx_queues)
> +		return -EINVAL;
> +
> +	peer_priv = netdev_priv(priv->peer);
> +	if (peer_priv->_xdp_prog && ch->tx_count > priv->peer->real_num_rx_queues)
> +		return -EINVAL;

An XDP program could be loaded later, so I think it's better to enforce
this constraint unconditionally.

(What happens on the regular xmit path if the number of TX queues is out
of sync with the peer RX queues, BTW?)

> +	if (netif_running(dev))
> +		veth_close(dev);
> +
> +	priv->num_tx_queues = ch->tx_count;
> +	priv->num_rx_queues = ch->rx_count;

Why can't just you use netif_set_real_num_*_queues() here directly (and
get rid of the priv members as above)?

-Toke

Re: [RFC PATCH 2/3] veth: make queues nr configurable via kernel module params

[Toke Høiland-Jørgensen]

Paolo Abeni <pabeni@redhat.com> writes:

> This allows configuring the number of tx and rx queues at
> module load time. A single module parameter controls
> both the default number of RX and TX queues created
> at device registration time.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  drivers/net/veth.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index 10360228a06a..787b4ad2cc87 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -27,6 +27,11 @@
>  #include <linux/bpf_trace.h>
>  #include <linux/net_tstamp.h>
>  
> +static int queues_nr	= 1;
> +
> +module_param(queues_nr, int, 0644);
> +MODULE_PARM_DESC(queues_nr, "Max number of RX and TX queues (default = 1)");

Adding new module parameters is generally discouraged. Also, it's sort
of a cumbersome API that you'll have to set this first, then re-create
the device, and then use channels to get the number you want.

So why not just default to allocating num_possible_cpus() number of
queues? Arguably that is the value that makes the most sense from a
scalability point of view anyway, but if we're concerned about behaviour
change (are we?), we could just default real_num_*_queues to 1, so that
the extra queues have to be explicitly enabled by ethtool?

-Toke

Re: [RFC PATCH 1/3] veth: implement support for set_channel ethtool op

[Paolo Abeni]

Hello,

On Fri, 2021-07-09 at 12:15 +0200, Toke Høiland-Jørgensen wrote:
> > @@ -224,10 +226,49 @@ static void veth_get_channels(struct net_device *dev,
> >  {
> >  	channels->tx_count = dev->real_num_tx_queues;
> >  	channels->rx_count = dev->real_num_rx_queues;
> > -	channels->max_tx = dev->real_num_tx_queues;
> > -	channels->max_rx = dev->real_num_rx_queues;
> > +	channels->max_tx = dev->num_tx_queues;
> > +	channels->max_rx = dev->num_rx_queues;
> >  	channels->combined_count = min(dev->real_num_rx_queues, dev->real_num_tx_queues);
> > -	channels->max_combined = min(dev->real_num_rx_queues, dev->real_num_tx_queues);
> > +	channels->max_combined = min(dev->num_rx_queues, dev->num_tx_queues);
> > +}
> > +
> > +static int veth_close(struct net_device *dev);
> > +static int veth_open(struct net_device *dev);
> > +
> > +static int veth_set_channels(struct net_device *dev,
> > +			     struct ethtool_channels *ch)
> > +{
> > +	struct veth_priv *priv = netdev_priv(dev);
> > +	struct veth_priv *peer_priv;
> > +
> > +	/* accept changes only on rx/tx */
> > +	if (ch->combined_count != min(dev->real_num_rx_queues, dev->real_num_tx_queues))
> > +		return -EINVAL;
> > +
> > +	/* respect contraint posed at device creation time */
> > +	if (ch->rx_count > dev->num_rx_queues || ch->tx_count > dev->num_tx_queues)
> > +		return -EINVAL;
> > +
> > +	if (!ch->rx_count || !ch->tx_count)
> > +		return -EINVAL;
> > +
> > +	/* avoid braking XDP, if that is enabled */
> > +	if (priv->_xdp_prog && ch->rx_count < priv->peer->real_num_tx_queues)
> > +		return -EINVAL;
> > +
> > +	peer_priv = netdev_priv(priv->peer);
> > +	if (peer_priv->_xdp_prog && ch->tx_count > priv->peer->real_num_rx_queues)
> > +		return -EINVAL;
> 
> An XDP program could be loaded later, so I think it's better to enforce
> this constraint unconditionally.

The relevant check is already present in veth_xdp_set(), the load will
be rejected with an appropriate extack message.

I enforcing the above contraint uncontiditionally will make impossible
changing the number of real queues at runtime, as we must update dev
and peer in different moments.

> (What happens on the regular xmit path if the number of TX queues is out
> of sync with the peer RX queues, BTW?)

if tx < rx, the higly number of rx queue will not be used, if rx < tx,
XDP/gro can't take place: the normal veth path is used.

> > +	if (netif_running(dev))
> > +		veth_close(dev);
> > +
> > +	priv->num_tx_queues = ch->tx_count;
> > +	priv->num_rx_queues = ch->rx_count;
> 
> Why can't just you use netif_set_real_num_*_queues() here directly (and
> get rid of the priv members as above)?

Uhm... I haven't thought about it. Let me try ;)

Thanks!

/P

Re: [RFC PATCH 1/3] veth: implement support for set_channel ethtool op

[Toke Høiland-Jørgensen]

Paolo Abeni <pabeni@redhat.com> writes:

> Hello,
>
> On Fri, 2021-07-09 at 12:15 +0200, Toke Høiland-Jørgensen wrote:
>> > @@ -224,10 +226,49 @@ static void veth_get_channels(struct net_device *dev,
>> >  {
>> >  	channels->tx_count = dev->real_num_tx_queues;
>> >  	channels->rx_count = dev->real_num_rx_queues;
>> > -	channels->max_tx = dev->real_num_tx_queues;
>> > -	channels->max_rx = dev->real_num_rx_queues;
>> > +	channels->max_tx = dev->num_tx_queues;
>> > +	channels->max_rx = dev->num_rx_queues;
>> >  	channels->combined_count = min(dev->real_num_rx_queues, dev->real_num_tx_queues);
>> > -	channels->max_combined = min(dev->real_num_rx_queues, dev->real_num_tx_queues);
>> > +	channels->max_combined = min(dev->num_rx_queues, dev->num_tx_queues);
>> > +}
>> > +
>> > +static int veth_close(struct net_device *dev);
>> > +static int veth_open(struct net_device *dev);
>> > +
>> > +static int veth_set_channels(struct net_device *dev,
>> > +			     struct ethtool_channels *ch)
>> > +{
>> > +	struct veth_priv *priv = netdev_priv(dev);
>> > +	struct veth_priv *peer_priv;
>> > +
>> > +	/* accept changes only on rx/tx */
>> > +	if (ch->combined_count != min(dev->real_num_rx_queues, dev->real_num_tx_queues))
>> > +		return -EINVAL;
>> > +
>> > +	/* respect contraint posed at device creation time */
>> > +	if (ch->rx_count > dev->num_rx_queues || ch->tx_count > dev->num_tx_queues)
>> > +		return -EINVAL;
>> > +
>> > +	if (!ch->rx_count || !ch->tx_count)
>> > +		return -EINVAL;
>> > +
>> > +	/* avoid braking XDP, if that is enabled */
>> > +	if (priv->_xdp_prog && ch->rx_count < priv->peer->real_num_tx_queues)
>> > +		return -EINVAL;
>> > +
>> > +	peer_priv = netdev_priv(priv->peer);
>> > +	if (peer_priv->_xdp_prog && ch->tx_count > priv->peer->real_num_rx_queues)
>> > +		return -EINVAL;
>> 
>> An XDP program could be loaded later, so I think it's better to enforce
>> this constraint unconditionally.
>
> The relevant check is already present in veth_xdp_set(), the load will
> be rejected with an appropriate extack message.

Ah, right, of course; silly me, that's fine then... :)

> I enforcing the above contraint uncontiditionally will make impossible
> changing the number of real queues at runtime, as we must update dev
> and peer in different moments.
>
>> (What happens on the regular xmit path if the number of TX queues is out
>> of sync with the peer RX queues, BTW?)
>
> if tx < rx, the higly number of rx queue will not be used, if rx < tx,
> XDP/gro can't take place: the normal veth path is used.

Right, OK, that's probably also fine then :)

-Toke

Re: [RFC PATCH 1/3] veth: implement support for set_channel ethtool op

[Paolo Abeni]

On Fri, 2021-07-09 at 12:49 +0200, Paolo Abeni wrote:
> On Fri, 2021-07-09 at 12:15 +0200, Toke Høiland-Jørgensen wrote:
> > > +	if (netif_running(dev))
> > > +		veth_close(dev);
> > > +
> > > +	priv->num_tx_queues = ch->tx_count;
> > > +	priv->num_rx_queues = ch->rx_count;
> > 
> > Why can't just you use netif_set_real_num_*_queues() here directly (and
> > get rid of the priv members as above)?
> 
> Uhm... I haven't thought about it. Let me try ;)

Here there is a possible problem: if the latter
netif_set_real_num_*_queues() fails, we should not change the current
configuration, so we should revert the
first netif_set_real_num_*_queues() change.

Even that additional revert operation could fail. If/when that happen
set_channel() will leave the device in a different state from both the
old one and the new one, possibly with an XDP-incompatible number of
queues.

Keeping the  netif_set_real_num_*_queues() calls in veth_open() avoid
the above issue: if the queue creation is problematic, the device will
stay down.

I think the additional fields are worthy, WDYT? 

Cheers,
Paolo

Re: [RFC PATCH 1/3] veth: implement support for set_channel ethtool op

[Toke Høiland-Jørgensen]

Paolo Abeni <pabeni@redhat.com> writes:

> On Fri, 2021-07-09 at 12:49 +0200, Paolo Abeni wrote:
>> On Fri, 2021-07-09 at 12:15 +0200, Toke Høiland-Jørgensen wrote:
>> > > +	if (netif_running(dev))
>> > > +		veth_close(dev);
>> > > +
>> > > +	priv->num_tx_queues = ch->tx_count;
>> > > +	priv->num_rx_queues = ch->rx_count;
>> > 
>> > Why can't just you use netif_set_real_num_*_queues() here directly (and
>> > get rid of the priv members as above)?
>> 
>> Uhm... I haven't thought about it. Let me try ;)
>
> Here there is a possible problem: if the latter
> netif_set_real_num_*_queues() fails, we should not change the current
> configuration, so we should revert the
> first netif_set_real_num_*_queues() change.
>
> Even that additional revert operation could fail. If/when that happen
> set_channel() will leave the device in a different state from both the
> old one and the new one, possibly with an XDP-incompatible number of
> queues.
>
> Keeping the  netif_set_real_num_*_queues() calls in veth_open() avoid
> the above issue: if the queue creation is problematic, the device will
> stay down.
>
> I think the additional fields are worthy, WDYT?

Hmm, wouldn't the right thing to do be to back out the change and return
an error to userspace? Something like:

+	if (netif_running(dev))
+		veth_close(dev);
+
+	old_rx_queues = dev->real_num_rx_queues;
+	err = netif_set_real_num_rx_queues(dev, ch->rx_count);
+	if (err)
+		return err;
+
+	err = netif_set_real_num_tx_queues(dev, ch->tx_count);
+	if (err) {
+		netif_set_real_num_rx_queues(dev, old_rx_queues);
+		return err;
+	}
+
+	if (netif_running(dev))
+		veth_open(dev);
+	return 0;


(also, shouldn't the result of veth_open() be returned? bit weird if you
don't get an error but the device stays down...)

-Toke

Re: [RFC PATCH 2/3] veth: make queues nr configurable via kernel module params

[Paolo Abeni]

On Fri, 2021-07-09 at 12:24 +0200, Toke Høiland-Jørgensen wrote:
> Paolo Abeni <pabeni@redhat.com> writes:
> 
> > This allows configuring the number of tx and rx queues at
> > module load time. A single module parameter controls
> > both the default number of RX and TX queues created
> > at device registration time.
> > 
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> >  drivers/net/veth.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> > index 10360228a06a..787b4ad2cc87 100644
> > --- a/drivers/net/veth.c
> > +++ b/drivers/net/veth.c
> > @@ -27,6 +27,11 @@
> >  #include <linux/bpf_trace.h>
> >  #include <linux/net_tstamp.h>
> >  
> > +static int queues_nr	= 1;
> > +
> > +module_param(queues_nr, int, 0644);
> > +MODULE_PARM_DESC(queues_nr, "Max number of RX and TX queues (default = 1)");
> 
> Adding new module parameters is generally discouraged. Also, it's sort
> of a cumbersome API that you'll have to set this first, then re-create
> the device, and then use channels to get the number you want.
> 
> So why not just default to allocating num_possible_cpus() number of
> queues? Arguably that is the value that makes the most sense from a
> scalability point of view anyway, but if we're concerned about behaviour
> change (are we?), we could just default real_num_*_queues to 1, so that
> the extra queues have to be explicitly enabled by ethtool?

I was concerned by the amount of memory wasted memory (should be ~256
bytes per rx queue, ~320 per tx, plus the sysfs entries).

real_num_tx_queue > 1 will makes the xmit path slower, so we likely
want to keep that to 1 by default - unless the userspace explicitly set
numtxqueues via netlink.

Finally, a default large num_tx_queue slows down device creation:

cat << ENDL > run.sh
#!/bin/sh
MAX=$1
for I in `seq 1 $MAX`; do
	ip link add name v$I type veth peer name pv$I
done
for I in `seq 1 $MAX`; do
	ip link del dev v$I
done
ENDL
chmod a+x run.sh

# with num_tx_queue == 1
time ./run.sh 100 
real	0m2.276s
user	0m0.107s
sys	0m0.162s

# with num_tx_queue == 128
time ./run.sh 100 1
real	0m4.199s
user	0m0.091s
sys	0m1.419s

# with num_tx_queue == 4096
time ./run.sh 100 
real	0m24.519s
user	0m0.089s
sys	0m21.711s

Still, if there is agreement I can switch to num_possible_cpus default,
plus some trickery to keep real_num_{r,t}x_queue unchanged.

WDYT?

Thanks!

Paolo

Re: [RFC PATCH 2/3] veth: make queues nr configurable via kernel module params

[Toke Høiland-Jørgensen]

Paolo Abeni <pabeni@redhat.com> writes:

> On Fri, 2021-07-09 at 12:24 +0200, Toke Høiland-Jørgensen wrote:
>> Paolo Abeni <pabeni@redhat.com> writes:
>> 
>> > This allows configuring the number of tx and rx queues at
>> > module load time. A single module parameter controls
>> > both the default number of RX and TX queues created
>> > at device registration time.
>> > 
>> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>> > ---
>> >  drivers/net/veth.c | 21 +++++++++++++++++++++
>> >  1 file changed, 21 insertions(+)
>> > 
>> > diff --git a/drivers/net/veth.c b/drivers/net/veth.c
>> > index 10360228a06a..787b4ad2cc87 100644
>> > --- a/drivers/net/veth.c
>> > +++ b/drivers/net/veth.c
>> > @@ -27,6 +27,11 @@
>> >  #include <linux/bpf_trace.h>
>> >  #include <linux/net_tstamp.h>
>> >  
>> > +static int queues_nr	= 1;
>> > +
>> > +module_param(queues_nr, int, 0644);
>> > +MODULE_PARM_DESC(queues_nr, "Max number of RX and TX queues (default = 1)");
>> 
>> Adding new module parameters is generally discouraged. Also, it's sort
>> of a cumbersome API that you'll have to set this first, then re-create
>> the device, and then use channels to get the number you want.
>> 
>> So why not just default to allocating num_possible_cpus() number of
>> queues? Arguably that is the value that makes the most sense from a
>> scalability point of view anyway, but if we're concerned about behaviour
>> change (are we?), we could just default real_num_*_queues to 1, so that
>> the extra queues have to be explicitly enabled by ethtool?
>
> I was concerned by the amount of memory wasted memory (should be ~256
> bytes per rx queue, ~320 per tx, plus the sysfs entries).

I'm not too worried by that since it's per CPU; systems with a lot of
CPUs should hopefully also have plenty of memory. Or at least I think
the user friendliness outweighs the cost in memory.

> real_num_tx_queue > 1 will makes the xmit path slower, so we likely
> want to keep that to 1 by default - unless the userspace explicitly set
> numtxqueues via netlink.

Right, that's fine by me :)

> Finally, a default large num_tx_queue slows down device creation:
>
> cat << ENDL > run.sh
> #!/bin/sh
> MAX=$1
> for I in `seq 1 $MAX`; do
> 	ip link add name v$I type veth peer name pv$I
> done
> for I in `seq 1 $MAX`; do
> 	ip link del dev v$I
> done
> ENDL
> chmod a+x run.sh
>
> # with num_tx_queue == 1
> time ./run.sh 100 
> real	0m2.276s
> user	0m0.107s
> sys	0m0.162s
>
> # with num_tx_queue == 128
> time ./run.sh 100 1
> real	0m4.199s
> user	0m0.091s
> sys	0m1.419s
>
> # with num_tx_queue == 4096
> time ./run.sh 100 
> real	0m24.519s
> user	0m0.089s
> sys	0m21.711s

So ~42 ms to create a device if there are 128 CPUs? And ~245 when
there's 4k CPUs? Doesn't seem too onerous to me...

> Still, if there is agreement I can switch to num_possible_cpus default,
> plus some trickery to keep real_num_{r,t}x_queue unchanged.
>
> WDYT?

SGTM :)

-Toke

Re: [RFC PATCH 1/3] veth: implement support for set_channel ethtool op

[Paolo Abeni]

On Fri, 2021-07-09 at 17:23 +0200, Toke Høiland-Jørgensen wrote:
> Paolo Abeni <pabeni@redhat.com> writes:
> > On Fri, 2021-07-09 at 12:49 +0200, Paolo Abeni wrote:
> > > On Fri, 2021-07-09 at 12:15 +0200, Toke Høiland-Jørgensen wrote:
> > > > > +	if (netif_running(dev))
> > > > > +		veth_close(dev);
> > > > > +
> > > > > +	priv->num_tx_queues = ch->tx_count;
> > > > > +	priv->num_rx_queues = ch->rx_count;
> > > > 
> > > > Why can't just you use netif_set_real_num_*_queues() here directly (and
> > > > get rid of the priv members as above)?
> > > 
> > > Uhm... I haven't thought about it. Let me try ;)
> > 
> > Here there is a possible problem: if the latter
> > netif_set_real_num_*_queues() fails, we should not change the current
> > configuration, so we should revert the
> > first netif_set_real_num_*_queues() change.
> > 
> > Even that additional revert operation could fail. If/when that happen
> > set_channel() will leave the device in a different state from both the
> > old one and the new one, possibly with an XDP-incompatible number of
> > queues.
> > 
> > Keeping the  netif_set_real_num_*_queues() calls in veth_open() avoid
> > the above issue: if the queue creation is problematic, the device will
> > stay down.
> > 
> > I think the additional fields are worthy, WDYT?
> 
> Hmm, wouldn't the right thing to do be to back out the change and return
> an error to userspace? Something like:
> 
> +	if (netif_running(dev))
> +		veth_close(dev);
> +
> +	old_rx_queues = dev->real_num_rx_queues;
> +	err = netif_set_real_num_rx_queues(dev, ch->rx_count);
> +	if (err)
> +		return err;
> +
> +	err = netif_set_real_num_tx_queues(dev, ch->tx_count);
> +	if (err) {
> +		netif_set_real_num_rx_queues(dev, old_rx_queues);

I'm sorry, I was not clear enough. I mean: even the
above netif_set_real_num_rx_queues() can fail. When that happen we will
leave the device in an inconsistent state, possibly even with an
"unsupported" queue setting.

> +		return err;
> +	}
> +
> +	if (netif_running(dev))
> +		veth_open(dev);
> +	return 0;
> 
> 
> (also, shouldn't the result of veth_open() be returned? bit weird if you
> don't get an error but the device stays down...)

Agreed.

Thanks!

Paolo

Re: [RFC PATCH 1/3] veth: implement support for set_channel ethtool op

[Jakub Kicinski]

On Fri,  9 Jul 2021 11:39:48 +0200 Paolo Abeni wrote:
> +	/* accept changes only on rx/tx */
> +	if (ch->combined_count != min(dev->real_num_rx_queues, dev->real_num_tx_queues))
> +		return -EINVAL;

Ah damn, I must have missed the get_channels being added. I believe the
correct interpretation of the params is rx means NAPI with just Rx
queue(s), tx NAPI with just Tx queue(s) and combined has both.
IOW combined != min(rx, tx).
Instead real_rx = combined + rx; real_tx = combined + tx.
Can we still change this?

> +	/* respect contraint posed at device creation time */
> +	if (ch->rx_count > dev->num_rx_queues || ch->tx_count > dev->num_tx_queues)
> +		return -EINVAL;

Could you lift this check into ethtool core?

> +	if (!ch->rx_count || !ch->tx_count)
> +		return -EINVAL;

You wouldn't need this with the right interpretation of combined :(

> +	/* avoid braking XDP, if that is enabled */
> +	if (priv->_xdp_prog && ch->rx_count < priv->peer->real_num_tx_queues)
> +		return -EINVAL;
> +
> +	peer_priv = netdev_priv(priv->peer);
> +	if (peer_priv->_xdp_prog && ch->tx_count > priv->peer->real_num_rx_queues)
> +		return -EINVAL;
> +
> +	if (netif_running(dev))
> +		veth_close(dev);
> +
> +	priv->num_tx_queues = ch->tx_count;
> +	priv->num_rx_queues = ch->rx_count;
> +
> +	if (netif_running(dev))
> +		veth_open(dev);

Re: [RFC PATCH 1/3] veth: implement support for set_channel ethtool op

[Jakub Kicinski]

On Fri, 09 Jul 2021 18:35:59 +0200 Paolo Abeni wrote:
> > +	if (netif_running(dev))
> > +		veth_open(dev);
> > +	return 0;
> > 
> > 
> > (also, shouldn't the result of veth_open() be returned? bit weird if you
> > don't get an error but the device stays down...)  
> 
> Agreed.

We've been fighting hard to make sure ethtool -L/-G/etc don't leave
devices half-broken. Maybe it's not as important for software devices,
but we should set a good example. We shouldn't take the device down
unless we are sure we'll be able to bring it up. So if veth_open()
"can't fail" it should be a WARN_ON(veth_open()), not return; and if 
it may fail due to memory allocations etc we should do a prepare/commit.

Re: [RFC PATCH 1/3] veth: implement support for set_channel ethtool op

[David Ahern]

On 7/9/21 1:54 PM, Jakub Kicinski wrote:
> On Fri,  9 Jul 2021 11:39:48 +0200 Paolo Abeni wrote:
>> +	/* accept changes only on rx/tx */
>> +	if (ch->combined_count != min(dev->real_num_rx_queues, dev->real_num_tx_queues))
>> +		return -EINVAL;
> 
> Ah damn, I must have missed the get_channels being added. I believe the
> correct interpretation of the params is rx means NAPI with just Rx
> queue(s), tx NAPI with just Tx queue(s) and combined has both.
> IOW combined != min(rx, tx).
> Instead real_rx = combined + rx; real_tx = combined + tx.
> Can we still change this?

Is it not an 'either' / 'or' situation? ie., you can either control the
number of Rx and Tx queues or you control the combined value but not
both. That is what I recall from nics (e.g., ConnectX).

Re: [RFC PATCH 1/3] veth: implement support for set_channel ethtool op

[Paolo Abeni]

Hello,

On Sun, 2021-07-11 at 19:44 -0600, David Ahern wrote:
> On 7/9/21 1:54 PM, Jakub Kicinski wrote:
> > On Fri,  9 Jul 2021 11:39:48 +0200 Paolo Abeni wrote:
> > > +	/* accept changes only on rx/tx */
> > > +	if (ch->combined_count != min(dev->real_num_rx_queues, dev->real_num_tx_queues))
> > > +		return -EINVAL;
> > 
> > Ah damn, I must have missed the get_channels being added. I believe the
> > correct interpretation of the params is rx means NAPI with just Rx
> > queue(s), tx NAPI with just Tx queue(s) and combined has both.
> > IOW combined != min(rx, tx).
> > Instead real_rx = combined + rx; real_tx = combined + tx.
> > Can we still change this?
> 
> Is it not an 'either' / 'or' situation? ie., you can either control the
> number of Rx and Tx queues or you control the combined value but not
> both. That is what I recall from nics (e.g., ConnectX).

Thanks for the feedback. My understanding was quite alike what David
stated - and indeed that is what ConnectX enforces AFAICS. Anyhow the
core ethtool code allows for what Jackub said, so I guess I need to
deal with that.

@Jakub: if we are still on time about changing the veth_get_channel()
exposed behaviour, what about just showing nr combined == 0 and
enforcing comined_max == 0? that would both describe more closely the
veth architecture and will make the code simpler - beyond fixing the
current uncorrect nr channels report.

Thanks!

Paolo

Re: [RFC PATCH 1/3] veth: implement support for set_channel ethtool op

[Jakub Kicinski]

On Mon, 12 Jul 2021 12:45:13 +0200 Paolo Abeni wrote:
> On Sun, 2021-07-11 at 19:44 -0600, David Ahern wrote:
> > On 7/9/21 1:54 PM, Jakub Kicinski wrote:  
> > > Ah damn, I must have missed the get_channels being added. I believe the
> > > correct interpretation of the params is rx means NAPI with just Rx
> > > queue(s), tx NAPI with just Tx queue(s) and combined has both.
> > > IOW combined != min(rx, tx).
> > > Instead real_rx = combined + rx; real_tx = combined + tx.
> > > Can we still change this?  
> > 
> > Is it not an 'either' / 'or' situation? ie., you can either control the
> > number of Rx and Tx queues or you control the combined value but not
> > both. That is what I recall from nics (e.g., ConnectX).  
> 
> Thanks for the feedback. My understanding was quite alike what David
> stated - and indeed that is what ConnectX enforces AFAICS. Anyhow the
> core ethtool code allows for what Jackub said, so I guess I need to
> deal with that.

I thought Mellanox was doing something funny to reuse the rx as the
number of AF_XDP queues. Normal rings are not reported twice, they're
only reported as combined.

ethtool man page is relatively clear, unfortunately the kernel code 
is not and few read the man page. A channel is approximately an IRQ, 
not a queue, and IRQ can't be dedicated and combined simultaneously:

 "A channel is an IRQ and the set of queues that can trigger that IRQ."

 " rx N   Changes the number of channels with only receive queues."

> @Jakub: if we are still on time about changing the veth_get_channel()
> exposed behaviour, what about just showing nr combined == 0 and
> enforcing comined_max == 0? that would both describe more closely the
> veth architecture and will make the code simpler - beyond fixing the
> current uncorrect nr channels report.

SGTM.