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

["Toke Høiland-Jørgensen" <toke@redhat.com>]

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