netdev - Re: [PATCH net-next] virtio_net: force_napi

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]

Date:   Tue, 24 Jul 2018 13:42:49 +0300
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Caleb Raitto <caleb.raitto@...il.com>
Cc:     jasowang@...hat.com, davem@...emloft.net, netdev@...r.kernel.org,
        Caleb Raitto <caraitto@...gle.com>
Subject: Re: [PATCH net-next] virtio_net: force_napi_tx module param.

On Mon, Jul 23, 2018 at 04:11:19PM -0700, Caleb Raitto wrote:
> From: Caleb Raitto <caraitto@...gle.com>
> 
> The driver disables tx napi if it's not certain that completions will
> be processed affine with tx service.
> 
> Its heuristic doesn't account for some scenarios where it is, such as
> when the queue pair count matches the core but not hyperthread count.
> 
> Allow userspace to override the heuristic. This is an alternative
> solution to that in the linked patch. That added more logic in the
> kernel for these cases, but the agreement was that this was better left
> to user control.
> 
> Do not expand the existing napi_tx variable to a ternary value,
> because doing so can break user applications that expect
> boolean ('Y'/'N') instead of integer output. Add a new param instead.
> 
> Link: https://patchwork.ozlabs.org/patch/725249/
> Acked-by: Willem de Bruijn <willemb@...gle.com>
> Acked-by: Jon Olson <jonolson@...gle.com>
> Signed-off-by: Caleb Raitto <caraitto@...gle.com>

Is there a reason the same rule should apply to all devices?
If not shouldn't this be an ethtool option?

> ---
>  drivers/net/virtio_net.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 2ff08bc103a9..d9aca4e90d6b 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -39,10 +39,11 @@
>  static int napi_weight = NAPI_POLL_WEIGHT;
>  module_param(napi_weight, int, 0444);
>  
> -static bool csum = true, gso = true, napi_tx;
> +static bool csum = true, gso = true, napi_tx, force_napi_tx;
>  module_param(csum, bool, 0444);
>  module_param(gso, bool, 0444);
>  module_param(napi_tx, bool, 0644);
> +module_param(force_napi_tx, bool, 0644);
>  
>  /* FIXME: MTU in config. */
>  #define GOOD_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
> @@ -1201,7 +1202,7 @@ static void virtnet_napi_tx_enable(struct virtnet_info *vi,
>  	/* Tx napi touches cachelines on the cpu handling tx interrupts. Only
>  	 * enable the feature if this is likely affine with the transmit path.
>  	 */
> -	if (!vi->affinity_hint_set) {
> +	if (!vi->affinity_hint_set && !force_napi_tx) {
>  		napi->weight = 0;
>  		return;
>  	}
> @@ -2646,7 +2647,7 @@ static int virtnet_alloc_queues(struct virtnet_info *vi)
>  		netif_napi_add(vi->dev, &vi->rq[i].napi, virtnet_poll,
>  			       napi_weight);
>  		netif_tx_napi_add(vi->dev, &vi->sq[i].napi, virtnet_poll_tx,
> -				  napi_tx ? napi_weight : 0);
> +				  (napi_tx || force_napi_tx) ? napi_weight : 0);
>  
>  		sg_init_table(vi->rq[i].sg, ARRAY_SIZE(vi->rq[i].sg));
>  		ewma_pkt_len_init(&vi->rq[i].mrg_avg_pkt_len);
> -- 
> 2.18.0.233.g985f88cf7e-goog