Re: [PATCH net-next RFC] virtio_net: ethtool tx napi configuration

["Tobin C. Harding" <me@tobin.cc>]

On Wed, Sep 12, 2018 at 07:29:11PM -0400, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
> 
> Implement ethtool .set_priv_flags and .get_priv_flags handlers
> and use ethtool private flags to toggle transmit napi:
> 
>   ethtool --set-priv-flags eth0 tx-napi on
>   ethtool --show-priv-flags eth0
> 
> Link: https://patchwork.ozlabs.org/patch/948149/
> Suggested-by: Jason Wang <jasowang@redhat.com>
> Suggested-by: Florian Fainelli <f.fainelli@gmail.com>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> ---
>  drivers/net/virtio_net.c | 49 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 765920905226..9ca7e0a0f0d9 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -73,6 +73,10 @@ static const unsigned long guest_offloads[] = {
>  	VIRTIO_NET_F_GUEST_UFO
>  };
>  
> +static const char virtnet_ethtool_priv_flags[][ETH_GSTRING_LEN] = {
> +	"tx-napi",
> +};
> +
>  struct virtnet_stat_desc {
>  	char desc[ETH_GSTRING_LEN];
>  	size_t offset;
> @@ -2059,6 +2063,9 @@ static void virtnet_get_strings(struct net_device *dev, u32 stringset, u8 *data)
>  			}
>  		}
>  		break;
> +	case ETH_SS_PRIV_FLAGS:
> +		memcpy(data, virtnet_ethtool_priv_flags,
> +		       sizeof(virtnet_ethtool_priv_flags));
>  	}
>  }
>  
> @@ -2070,6 +2077,9 @@ static int virtnet_get_sset_count(struct net_device *dev, int sset)
>  	case ETH_SS_STATS:
>  		return vi->curr_queue_pairs * (VIRTNET_RQ_STATS_LEN +
>  					       VIRTNET_SQ_STATS_LEN);
> +	case ETH_SS_PRIV_FLAGS:
> +		return ARRAY_SIZE(virtnet_ethtool_priv_flags);
> +
>  	default:
>  		return -EOPNOTSUPP;
>  	}
> @@ -2181,6 +2191,43 @@ static int virtnet_get_link_ksettings(struct net_device *dev,
>  	return 0;
>  }
>  
> +static int virtnet_set_priv_flags(struct net_device *dev, u32 priv_flags)
> +{
> +	struct virtnet_info *vi = netdev_priv(dev);
> +	int i, napi_weight;
> +
> +	napi_weight = priv_flags & 0x1 ? NAPI_POLL_WEIGHT : 0;
> +
> +	if (napi_weight ^ vi->sq[0].napi.weight) {
> +		for (i = 0; i < vi->max_queue_pairs; i++) {
> +			struct netdev_queue *txq =
> +				netdev_get_tx_queue(vi->dev, i);
> +
> +			virtnet_napi_tx_disable(&vi->sq[i].napi);
> +			__netif_tx_lock_bh(txq);
> +			vi->sq[i].napi.weight = napi_weight;
> +			if (!napi_weight)
> +				virtqueue_enable_cb(vi->sq[i].vq);
> +			__netif_tx_unlock_bh(txq);
> +			virtnet_napi_tx_enable(vi, vi->sq[i].vq,
> +					       &vi->sq[i].napi);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static u32 virtnet_get_priv_flags(struct net_device *dev)
> +{
> +	struct virtnet_info *vi = netdev_priv(dev);
> +	int priv_flags = 0;
> +
> +	if (vi->sq[0].napi.weight)
> +		priv_flags |= 0x1;
> +
> +	return priv_flags;
> +}

Why the use of priv_flags here?  Is there some reason that we don't want
to use the more simple

    static u32 virtnet_get_priv_flags(struct net_device *dev)
    {
            struct virtnet_info *vi = netdev_priv(dev);
    
    	    if (vi->sq[0].napi.weight)
       	            return 1;
    
    	    return 0;
    }


thanks,
Tobin.