netdev - Re: [PATCH v2] net: virtio_net: notifications coalescing 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: <20220713200203.4eb3a64e@kernel.org>
Date:   Wed, 13 Jul 2022 20:02:03 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Alvaro Karsz <alvaro.karsz@...id-run.com>
Cc:     netdev@...r.kernel.org, "Michael S. Tsirkin" <mst@...hat.com>,
        Jason Wang <jasowang@...hat.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH v2] net: virtio_net: notifications coalescing support

On Tue, 12 Jul 2022 14:22:09 +0300 Alvaro Karsz wrote:
> @@ -2594,19 +2600,76 @@ static int virtnet_set_coalesce(struct net_device *dev,
>  {
>  	struct virtnet_info *vi = netdev_priv(dev);
>  	int i, napi_weight;
> +	struct scatterlist sgs_tx, sgs_rx;
> +	struct virtio_net_ctrl_coal_tx coal_tx;
> +	struct virtio_net_ctrl_coal_rx coal_rx;
> +	bool update_napi,
> +	notf_coal = virtio_has_feature(vi->vdev, VIRTIO_NET_F_NOTF_COAL);
> +
> +	/* rx_coalesce_usecs/tx_coalesce_usecs are supported only
> +	 * if VIRTIO_NET_F_NOTF_COAL feature is negotiated.
> +	 */
> +	if (!notf_coal && (ec->rx_coalesce_usecs || ec->tx_coalesce_usecs))
> +		return -EOPNOTSUPP;
> +
> +	if (notf_coal) {
> +		coal_tx.tx_usecs = cpu_to_le32(ec->tx_coalesce_usecs);
> +		coal_tx.tx_max_packets = cpu_to_le32(ec->tx_max_coalesced_frames);
> +		sg_init_one(&sgs_tx, &coal_tx, sizeof(coal_tx));
> +
> +		if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_NOTF_COAL,
> +					  VIRTIO_NET_CTRL_NOTF_COAL_TX_SET,
> +					  &sgs_tx))
> +			return -EINVAL;
> +
> +		/* Save parameters */
> +		vi->tx_usecs = ec->tx_coalesce_usecs;
> +		vi->tx_max_packets = ec->tx_max_coalesced_frames;
> +
> +		coal_rx.rx_usecs = cpu_to_le32(ec->rx_coalesce_usecs);
> +		coal_rx.rx_max_packets = cpu_to_le32(ec->rx_max_coalesced_frames);
> +		sg_init_one(&sgs_rx, &coal_rx, sizeof(coal_rx));
> +
> +		if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_NOTF_COAL,
> +					  VIRTIO_NET_CTRL_NOTF_COAL_RX_SET,
> +					  &sgs_rx))
> +			return -EINVAL;
> +
> +		/* Save parameters */
> +		vi->rx_usecs = ec->rx_coalesce_usecs;
> +		vi->rx_max_packets = ec->rx_max_coalesced_frames;
> +	}
> +
> +	/* Should we update NAPI? */
> +	update_napi = ec->tx_max_coalesced_frames <= 1 &&
> +			ec->rx_max_coalesced_frames == 1;

I'd vote for either interpreting the parameters in the new way (real
coalescing) or old way (NAPI update) but not both.

> -	if (ec->tx_max_coalesced_frames > 1 ||
> -	    ec->rx_max_coalesced_frames != 1)
> +	/* If notifications coalesing feature is not negotiated,
> +	 * and we can't update NAPI, return an error
> +	 */
> +	if (!notf_coal && !update_napi)
>  		return -EINVAL;
>  
> -	napi_weight = ec->tx_max_coalesced_frames ? NAPI_POLL_WEIGHT : 0;
> -	if (napi_weight ^ vi->sq[0].napi.weight) {
> -		if (dev->flags & IFF_UP)
> -			return -EBUSY;
> -		for (i = 0; i < vi->max_queue_pairs; i++)
> -			vi->sq[i].napi.weight = napi_weight;
> +	if (update_napi) {
> +		napi_weight = ec->tx_max_coalesced_frames ? NAPI_POLL_WEIGHT : 0;
> +		if (napi_weight ^ vi->sq[0].napi.weight) {
> +			if (dev->flags & IFF_UP) {
> +				/* If notifications coalescing feature is not negotiated,
> +				 * return an error, otherwise exit without changing
> +				 * the NAPI parameters.
> +				 */
> +				if (!notf_coal)
> +					return -EBUSY;
> +
> +				goto exit;
> +			}
> +
> +			for (i = 0; i < vi->max_queue_pairs; i++)
> +				vi->sq[i].napi.weight = napi_weight;
> +		}
>  	}
>  
> +exit:
>  	return 0;
>  }

This function got long now, and large parts of it are under if()s
Feels like you'd be better of factoring out the USEC update and the
existing logic into separate functions and calling them as needed.

> @@ -2616,14 +2679,25 @@ static int virtnet_get_coalesce(struct net_device *dev,
>  				struct netlink_ext_ack *extack)
>  {
>  	struct ethtool_coalesce ec_default = {
> -		.cmd = ETHTOOL_GCOALESCE,
> -		.rx_max_coalesced_frames = 1,
> +		.cmd = ETHTOOL_GCOALESCE
>  	};

I think the structure was here for conciseness, since you're not
populating it now just remove it and write to ec directly. 
ec->cmd does not have to be written it's already set.

> +
>  	struct virtnet_info *vi = netdev_priv(dev);
> +	bool notf_coal = virtio_has_feature(vi->vdev, VIRTIO_NET_F_NOTF_COAL);
> +
> +	/* Add notifications coalescing settings */
> +	if (notf_coal) {
> +		ec_default.rx_coalesce_usecs = vi->rx_usecs;
> +		ec_default.tx_coalesce_usecs = vi->tx_usecs;
> +		ec_default.tx_max_coalesced_frames = vi->tx_max_packets;
> +		ec_default.rx_max_coalesced_frames = vi->rx_max_packets;
> +	} else {
> +		ec_default.rx_max_coalesced_frames = 1;
> +	}


> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> index 3f55a4215f1..29ced55514d 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h

Is it typical for virtio to add the structures to uAPI even tho the
kernel does not consume them? I presume so, just wanted to flag it.