linux-kernel - Re: [PATCH net-next 1/1] net: stmmac: add per-queue TX & RX coalesce ethtool support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20210315125059.32fde79a@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 15 Mar 2021 12:50:59 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Ong Boon Leong <boon.leong.ong@...el.com>
Cc:     Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...com>,
        Jose Abreu <joabreu@...opsys.com>,
        "David S . Miller" <davem@...emloft.net>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        netdev@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 1/1] net: stmmac: add per-queue TX & RX
 coalesce ethtool support

On Mon, 15 Mar 2021 14:44:48 +0800 Ong Boon Leong wrote:
> Extending the driver to support per-queue RX and TX coalesce settings in
> order to support below commands:
> 
> To show per-queue coalesce setting:-
>  $ ethtool --per-queue <DEVNAME> queue_mask <MASK> --show-coalesce
> 
> To set per-queue coalesce setting:-
>  $ ethtool --per-queue <DEVNAME> queue_mask <MASK> --coalesce \
>      [rx-usecs N] [rx-frames M] [tx-usecs P] [tx-frames Q]
> 
> Signed-off-by: Ong Boon Leong <boon.leong.ong@...el.com>

> -static int stmmac_get_coalesce(struct net_device *dev,
> -			       struct ethtool_coalesce *ec)
> +static int __stmmac_get_coalesce(struct net_device *dev,
> +				 struct ethtool_coalesce *ec,
> +				 int queue)
>  {
>  	struct stmmac_priv *priv = netdev_priv(dev);
> +	u32 max_cnt;
> +	u32 rx_cnt;
> +	u32 tx_cnt;
>  
> -	ec->tx_coalesce_usecs = priv->tx_coal_timer;
> -	ec->tx_max_coalesced_frames = priv->tx_coal_frames;
> +	rx_cnt = priv->plat->rx_queues_to_use;
> +	tx_cnt = priv->plat->tx_queues_to_use;
> +	max_cnt = max(rx_cnt, tx_cnt);
>  
> -	if (priv->use_riwt) {
> -		ec->rx_max_coalesced_frames = priv->rx_coal_frames;
> -		ec->rx_coalesce_usecs = stmmac_riwt2usec(priv->rx_riwt, priv);
> +	if (queue < 0)
> +		queue = 0;
> +	else if (queue >= max_cnt)
> +		return -EINVAL;
> +
> +	if (queue < tx_cnt) {
> +		ec->tx_coalesce_usecs = priv->tx_coal_timer[queue];
> +		ec->tx_max_coalesced_frames = priv->tx_coal_frames[queue];
> +	} else {
> +		ec->tx_coalesce_usecs = -1;
> +		ec->tx_max_coalesced_frames = -1;
> +	}
> +
> +	if (priv->use_riwt && queue < rx_cnt) {
> +		ec->rx_max_coalesced_frames = priv->rx_coal_frames[queue];
> +		ec->rx_coalesce_usecs = stmmac_riwt2usec(priv->rx_riwt[queue],
> +							 priv);
> +	} else {
> +		ec->rx_max_coalesced_frames = -1;
> +		ec->rx_coalesce_usecs = -1;

Why the use of negative values? why not leave them as 0?

>  	}
>  
>  	return 0;
>  }
>  
> -static int stmmac_set_coalesce(struct net_device *dev,
> +static int stmmac_get_coalesce(struct net_device *dev,
>  			       struct ethtool_coalesce *ec)
> +{
> +	return __stmmac_get_coalesce(dev, ec, -1);
> +}
> +
> +static int stmmac_get_per_queue_coalesce(struct net_device *dev, u32 queue,
> +					 struct ethtool_coalesce *ec)
> +{
> +	return __stmmac_get_coalesce(dev, ec, queue);
> +}
> +
> +static int __stmmac_set_coalesce(struct net_device *dev,
> +				 struct ethtool_coalesce *ec,
> +				 int queue)
>  {
>  	struct stmmac_priv *priv = netdev_priv(dev);
> -	u32 rx_cnt = priv->plat->rx_queues_to_use;
> +	bool all_queues = false;
>  	unsigned int rx_riwt;
> +	u32 max_cnt;
> +	u32 rx_cnt;
> +	u32 tx_cnt;
> +
> +	rx_cnt = priv->plat->rx_queues_to_use;
> +	tx_cnt = priv->plat->tx_queues_to_use;
> +	max_cnt = max(rx_cnt, tx_cnt);
> +
> +	if (queue < 0)
> +		all_queues = true;
> +	else if (queue >= max_cnt)
> +		return -EINVAL;
> +
> +	/* Check not supported parameters  */
> +	if (ec->rx_coalesce_usecs_irq ||
> +	    ec->rx_max_coalesced_frames_irq || ec->tx_coalesce_usecs_irq ||
> +	    ec->use_adaptive_rx_coalesce || ec->use_adaptive_tx_coalesce ||
> +	    ec->pkt_rate_low || ec->rx_coalesce_usecs_low ||
> +	    ec->rx_max_coalesced_frames_low || ec->tx_coalesce_usecs_high ||
> +	    ec->tx_max_coalesced_frames_low || ec->pkt_rate_high ||
> +	    ec->tx_coalesce_usecs_low || ec->rx_coalesce_usecs_high ||
> +	    ec->rx_max_coalesced_frames_high ||
> +	    ec->tx_max_coalesced_frames_irq ||
> +	    ec->stats_block_coalesce_usecs ||
> +	    ec->tx_max_coalesced_frames_high || ec->rate_sample_interval)
> +		return -EOPNOTSUPP;

This shouldn't be needed now that supporter types are expressed in 
dev->ethtool_ops->supported_coalesce_params, no?

>  	if (priv->use_riwt && (ec->rx_coalesce_usecs > 0)) {
>  		rx_riwt = stmmac_usec2riwt(ec->rx_coalesce_usecs, priv);
> @@ -785,8 +846,23 @@ static int stmmac_set_coalesce(struct net_device *dev,
>  		if ((rx_riwt > MAX_DMA_RIWT) || (rx_riwt < MIN_DMA_RIWT))
>  			return -EINVAL;
>  
> -		priv->rx_riwt = rx_riwt;
> -		stmmac_rx_watchdog(priv, priv->ioaddr, priv->rx_riwt, rx_cnt);
> +		if (all_queues) {
> +			int i;
> +
> +			for (i = 0; i < rx_cnt; i++) {
> +				priv->rx_riwt[i] = rx_riwt;
> +				stmmac_rx_watchdog(priv, priv->ioaddr,
> +						   rx_riwt, i);
> +				priv->rx_coal_frames[i] =
> +					ec->rx_max_coalesced_frames;
> +			}
> +		} else if (queue < rx_cnt) {
> +			priv->rx_riwt[queue] = rx_riwt;
> +			stmmac_rx_watchdog(priv, priv->ioaddr,
> +					   rx_riwt, queue);
> +			priv->rx_coal_frames[queue] =
> +				ec->rx_max_coalesced_frames;
> +		}
>  	}
>  
>  	if ((ec->tx_coalesce_usecs == 0) &&
> @@ -797,13 +873,37 @@ static int stmmac_set_coalesce(struct net_device *dev,
>  	    (ec->tx_max_coalesced_frames > STMMAC_TX_MAX_FRAMES))
>  		return -EINVAL;
>  
> -	/* Only copy relevant parameters, ignore all others. */
> -	priv->tx_coal_frames = ec->tx_max_coalesced_frames;
> -	priv->tx_coal_timer = ec->tx_coalesce_usecs;
> -	priv->rx_coal_frames = ec->rx_max_coalesced_frames;
> +	if (all_queues) {
> +		int i;
> +
> +		for (i = 0; i < tx_cnt; i++) {
> +			priv->tx_coal_frames[i] =
> +				ec->tx_max_coalesced_frames;
> +			priv->tx_coal_timer[i] =
> +				ec->tx_coalesce_usecs;
> +		}
> +	} else if (queue < tx_cnt) {
> +		priv->tx_coal_frames[queue] =
> +			ec->tx_max_coalesced_frames;
> +		priv->tx_coal_timer[queue] =
> +			ec->tx_coalesce_usecs;
> +	}
> +
>  	return 0;
>  }