[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 2/3] veth: make queues nr configurable via kernel module params

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2267 bytes --]

Hi Paolo,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on net/master]
[also build test WARNING on net-next/master ipvs/master linus/master v5.13 next-20210709]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Paolo-Abeni/veth-more-flexible-channels-number-configuration/20210709-174314
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 2b452550a203d88112eaf0ba9fc4b750a000b496
config: parisc-randconfig-r011-20210709 (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/6256f5f16e2e81592bafc2b33401c86591362c89
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Paolo-Abeni/veth-more-flexible-channels-number-configuration/20210709-174314
        git checkout 6256f5f16e2e81592bafc2b33401c86591362c89
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=parisc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/net/veth.c:1670:14: warning: no previous prototype for 'veth_get_num_tx_queues' [-Wmissing-prototypes]
    1670 | unsigned int veth_get_num_tx_queues(void)
         |              ^~~~~~~~~~~~~~~~~~~~~~


vim +/veth_get_num_tx_queues +1670 drivers/net/veth.c

  1669	
> 1670	unsigned int veth_get_num_tx_queues(void)
  1671	{
  1672		/* enforce the same queue limit as rtnl_create_link */
  1673		int queues = queues_nr;
  1674	
  1675		if (queues < 1)
  1676			queues = 1;
  1677		if (queues > 4096)
  1678			queues = 4096;
  1679		return queues;
  1680	}
  1681	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 39882 bytes --]

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 2/3] veth: make queues nr configurable via kernel module params

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 8834 bytes --]

Hi Paolo,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on net/master]
[also build test WARNING on net-next/master ipvs/master linus/master v5.13 next-20210709]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Paolo-Abeni/veth-more-flexible-channels-number-configuration/20210709-174314
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 2b452550a203d88112eaf0ba9fc4b750a000b496
config: powerpc-randconfig-r021-20210709 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 8d69635ed9ecf36fd0ca85906bfde17949671cbe)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc cross compiling tool for clang build
        # apt-get install binutils-powerpc-linux-gnu
        # https://github.com/0day-ci/linux/commit/6256f5f16e2e81592bafc2b33401c86591362c89
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Paolo-Abeni/veth-more-flexible-channels-number-configuration/20210709-174314
        git checkout 6256f5f16e2e81592bafc2b33401c86591362c89
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   __do_insb
   ^
   arch/powerpc/include/asm/io.h:556:56: note: expanded from macro '__do_insb'
   #define __do_insb(p, b, n)      readsb((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
                                          ~~~~~~~~~~~~~~~~~~~~~^
   In file included from drivers/net/veth.c:12:
   In file included from include/linux/netdevice.h:37:
   In file included from include/net/net_namespace.h:39:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/powerpc/include/asm/io.h:619:
   arch/powerpc/include/asm/io-defs.h:45:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(insw, (unsigned long p, void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:113:1: note: expanded from here
   __do_insw
   ^
   arch/powerpc/include/asm/io.h:557:56: note: expanded from macro '__do_insw'
   #define __do_insw(p, b, n)      readsw((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
                                          ~~~~~~~~~~~~~~~~~~~~~^
   In file included from drivers/net/veth.c:12:
   In file included from include/linux/netdevice.h:37:
   In file included from include/net/net_namespace.h:39:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/powerpc/include/asm/io.h:619:
   arch/powerpc/include/asm/io-defs.h:47:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(insl, (unsigned long p, void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:115:1: note: expanded from here
   __do_insl
   ^
   arch/powerpc/include/asm/io.h:558:56: note: expanded from macro '__do_insl'
   #define __do_insl(p, b, n)      readsl((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
                                          ~~~~~~~~~~~~~~~~~~~~~^
   In file included from drivers/net/veth.c:12:
   In file included from include/linux/netdevice.h:37:
   In file included from include/net/net_namespace.h:39:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/powerpc/include/asm/io.h:619:
   arch/powerpc/include/asm/io-defs.h:49:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(outsb, (unsigned long p, const void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:117:1: note: expanded from here
   __do_outsb
   ^
   arch/powerpc/include/asm/io.h:559:58: note: expanded from macro '__do_outsb'
   #define __do_outsb(p, b, n)     writesb((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
                                           ~~~~~~~~~~~~~~~~~~~~~^
   In file included from drivers/net/veth.c:12:
   In file included from include/linux/netdevice.h:37:
   In file included from include/net/net_namespace.h:39:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/powerpc/include/asm/io.h:619:
   arch/powerpc/include/asm/io-defs.h:51:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(outsw, (unsigned long p, const void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:119:1: note: expanded from here
   __do_outsw
   ^
   arch/powerpc/include/asm/io.h:560:58: note: expanded from macro '__do_outsw'
   #define __do_outsw(p, b, n)     writesw((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
                                           ~~~~~~~~~~~~~~~~~~~~~^
   In file included from drivers/net/veth.c:12:
   In file included from include/linux/netdevice.h:37:
   In file included from include/net/net_namespace.h:39:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/powerpc/include/asm/io.h:619:
   arch/powerpc/include/asm/io-defs.h:53:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(outsl, (unsigned long p, const void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:121:1: note: expanded from here
   __do_outsl
   ^
   arch/powerpc/include/asm/io.h:561:58: note: expanded from macro '__do_outsl'
   #define __do_outsl(p, b, n)     writesl((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
                                           ~~~~~~~~~~~~~~~~~~~~~^
>> drivers/net/veth.c:1670:14: warning: no previous prototype for function 'veth_get_num_tx_queues' [-Wmissing-prototypes]
   unsigned int veth_get_num_tx_queues(void)
                ^
   drivers/net/veth.c:1670:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   unsigned int veth_get_num_tx_queues(void)
   ^
   static 
   14 warnings generated.


vim +/veth_get_num_tx_queues +1670 drivers/net/veth.c

  1669	
> 1670	unsigned int veth_get_num_tx_queues(void)
  1671	{
  1672		/* enforce the same queue limit as rtnl_create_link */
  1673		int queues = queues_nr;
  1674	
  1675		if (queues < 1)
  1676			queues = 1;
  1677		if (queues > 4096)
  1678			queues = 4096;
  1679		return queues;
  1680	}
  1681	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 30830 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3408 bytes --]

Hi Paolo,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on net/master]
[also build test WARNING on net-next/master ipvs/master linus/master v5.13 next-20210709]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Paolo-Abeni/veth-more-flexible-channels-number-configuration/20210709-174314
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 2b452550a203d88112eaf0ba9fc4b750a000b496
config: x86_64-rhel-8.3-kselftests (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-341-g8af24329-dirty
        # https://github.com/0day-ci/linux/commit/e778921949f8b7c673639dbf5bcf7d4f22b7567b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Paolo-Abeni/veth-more-flexible-channels-number-configuration/20210709-174314
        git checkout e778921949f8b7c673639dbf5bcf7d4f22b7567b
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> drivers/net/veth.c:259:37: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct net_device const *dev @@     got struct net_device [noderef] __rcu *peer @@
   drivers/net/veth.c:259:37: sparse:     expected struct net_device const *dev
   drivers/net/veth.c:259:37: sparse:     got struct net_device [noderef] __rcu *peer
>> drivers/net/veth.c:256:51: sparse: sparse: dereference of noderef expression
   drivers/net/veth.c:260:56: sparse: sparse: dereference of noderef expression

vim +259 drivers/net/veth.c

   237	
   238	static int veth_set_channels(struct net_device *dev,
   239				     struct ethtool_channels *ch)
   240	{
   241		struct veth_priv *priv = netdev_priv(dev);
   242		struct veth_priv *peer_priv;
   243	
   244		/* accept changes only on rx/tx */
   245		if (ch->combined_count != min(dev->real_num_rx_queues, dev->real_num_tx_queues))
   246			return -EINVAL;
   247	
   248		/* respect contraint posed at device creation time */
   249		if (ch->rx_count > dev->num_rx_queues || ch->tx_count > dev->num_tx_queues)
   250			return -EINVAL;
   251	
   252		if (!ch->rx_count || !ch->tx_count)
   253			return -EINVAL;
   254	
   255		/* avoid braking XDP, if that is enabled */
 > 256		if (priv->_xdp_prog && ch->rx_count < priv->peer->real_num_tx_queues)
   257			return -EINVAL;
   258	
 > 259		peer_priv = netdev_priv(priv->peer);
   260		if (peer_priv->_xdp_prog && ch->tx_count > priv->peer->real_num_rx_queues)
   261			return -EINVAL;
   262	
   263		if (netif_running(dev))
   264			veth_close(dev);
   265	
   266		priv->num_tx_queues = ch->tx_count;
   267		priv->num_rx_queues = ch->rx_count;
   268	
   269		if (netif_running(dev))
   270			veth_open(dev);
   271		return 0;
   272	}
   273	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 42042 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2323 bytes --]

Hi Paolo,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on net/master]
[also build test WARNING on net-next/master ipvs/master linus/master v5.13 next-20210709]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Paolo-Abeni/veth-more-flexible-channels-number-configuration/20210709-174314
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 2b452550a203d88112eaf0ba9fc4b750a000b496
config: x86_64-rhel-8.3-kselftests (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-341-g8af24329-dirty
        # https://github.com/0day-ci/linux/commit/6256f5f16e2e81592bafc2b33401c86591362c89
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Paolo-Abeni/veth-more-flexible-channels-number-configuration/20210709-174314
        git checkout 6256f5f16e2e81592bafc2b33401c86591362c89
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
   drivers/net/veth.c:264:37: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct net_device const *dev @@     got struct net_device [noderef] __rcu *peer @@
   drivers/net/veth.c:264:37: sparse:     expected struct net_device const *dev
   drivers/net/veth.c:264:37: sparse:     got struct net_device [noderef] __rcu *peer
>> drivers/net/veth.c:1670:14: sparse: sparse: symbol 'veth_get_num_tx_queues' was not declared. Should it be static?
   drivers/net/veth.c:261:51: sparse: sparse: dereference of noderef expression
   drivers/net/veth.c:265:56: sparse: sparse: dereference of noderef expression

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 42042 bytes --]

[RFC PATCH] veth: veth_get_num_tx_queues() can be static

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 758 bytes --]

drivers/net/veth.c:1670:14: warning: symbol 'veth_get_num_tx_queues' was not declared. Should it be static?

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: kernel test robot <lkp@intel.com>
---
 veth.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 787b4ad2cc876..3cad4d04e5cb9 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -1667,7 +1667,7 @@ 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)
+static unsigned int veth_get_num_tx_queues(void)
 {
 	/* enforce the same queue limit as rtnl_create_link */
 	int queues = queues_nr;

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.