Re: [PATCH v2 10/12] virtio: add Tx checksum offload support

[Maxime Coquelin <maxime.coquelin@redhat.com>]

Hi Olivier,

On 10/03/2016 11:00 AM, Olivier Matz wrote:
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c |  7 +++++
>  drivers/net/virtio/virtio_ethdev.h |  1 +
>  drivers/net/virtio/virtio_rxtx.c   | 57 +++++++++++++++++++++++++-------------
>  3 files changed, 45 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 43cb096..55024cd 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1578,6 +1578,13 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
>  	dev_info->rx_offload_capa =
>  		DEV_RX_OFFLOAD_TCP_CKSUM |
>  		DEV_RX_OFFLOAD_UDP_CKSUM;
> +	dev_info->tx_offload_capa = 0;
> +
> +	if (hw->guest_features & (1ULL << VIRTIO_NET_F_CSUM)) {
> +		dev_info->tx_offload_capa |=
> +			DEV_TX_OFFLOAD_UDP_CKSUM |
> +			DEV_TX_OFFLOAD_TCP_CKSUM;
> +	}
>  }
>
>  /*
> diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
> index 2fc9218..202aa2e 100644
> --- a/drivers/net/virtio/virtio_ethdev.h
> +++ b/drivers/net/virtio/virtio_ethdev.h
> @@ -62,6 +62,7 @@
>  	 1u << VIRTIO_NET_F_CTRL_VQ	  |	\
>  	 1u << VIRTIO_NET_F_CTRL_RX	  |	\
>  	 1u << VIRTIO_NET_F_CTRL_VLAN	  |	\
> +	 1u << VIRTIO_NET_F_CSUM	  |	\
>  	 1u << VIRTIO_NET_F_MRG_RXBUF	  |	\
>  	 1ULL << VIRTIO_F_VERSION_1)
>
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index eda678a..4ae11e7 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -213,13 +213,14 @@ static inline void
>  virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct rte_mbuf *cookie,
>  		       uint16_t needed, int use_indirect, int can_push)
>  {
> +	struct virtio_tx_region *txr = txvq->virtio_net_hdr_mz->addr;
>  	struct vq_desc_extra *dxp;
>  	struct virtqueue *vq = txvq->vq;
>  	struct vring_desc *start_dp;
>  	uint16_t seg_num = cookie->nb_segs;
>  	uint16_t head_idx, idx;
>  	uint16_t head_size = vq->hw->vtnet_hdr_size;
> -	unsigned long offs;
> +	struct virtio_net_hdr *hdr;
>
>  	head_idx = vq->vq_desc_head_idx;
>  	idx = head_idx;
> @@ -230,10 +231,9 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct rte_mbuf *cookie,
>  	start_dp = vq->vq_ring.desc;
>
>  	if (can_push) {
> -		/* put on zero'd transmit header (no offloads) */
> -		void *hdr = rte_pktmbuf_prepend(cookie, head_size);
> -
> -		memset(hdr, 0, head_size);
> +		/* prepend cannot fail, checked by caller */
> +		hdr = (struct virtio_net_hdr *)
> +			rte_pktmbuf_prepend(cookie, head_size);
>  	} else if (use_indirect) {
>  		/* setup tx ring slot to point to indirect
>  		 * descriptor list stored in reserved region.
> @@ -241,14 +241,11 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct rte_mbuf *cookie,
>  		 * the first slot in indirect ring is already preset
>  		 * to point to the header in reserved region
>  		 */
> -		struct virtio_tx_region *txr = txvq->virtio_net_hdr_mz->addr;
> -
> -		offs = idx * sizeof(struct virtio_tx_region)
> -			+ offsetof(struct virtio_tx_region, tx_indir);
> -
> -		start_dp[idx].addr  = txvq->virtio_net_hdr_mem + offs;
> +		start_dp[idx].addr  = txvq->virtio_net_hdr_mem +
> +			RTE_PTR_DIFF(&txr[idx].tx_indir, txr);
>  		start_dp[idx].len   = (seg_num + 1) * sizeof(struct vring_desc);
>  		start_dp[idx].flags = VRING_DESC_F_INDIRECT;
> +		hdr = (struct virtio_net_hdr *)&txr[idx].tx_hdr;
>
>  		/* loop below will fill in rest of the indirect elements */
>  		start_dp = txr[idx].tx_indir;
> @@ -257,15 +254,40 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct rte_mbuf *cookie,
>  		/* setup first tx ring slot to point to header
>  		 * stored in reserved region.
>  		 */
> -		offs = idx * sizeof(struct virtio_tx_region)
> -			+ offsetof(struct virtio_tx_region, tx_hdr);
> -
> -		start_dp[idx].addr  = txvq->virtio_net_hdr_mem + offs;
> +		start_dp[idx].addr  = txvq->virtio_net_hdr_mem +
> +			RTE_PTR_DIFF(&txr[idx].tx_hdr, txr);
>  		start_dp[idx].len   = vq->hw->vtnet_hdr_size;
>  		start_dp[idx].flags = VRING_DESC_F_NEXT;
> +		hdr = (struct virtio_net_hdr *)&txr[idx].tx_hdr;
> +
>  		idx = start_dp[idx].next;
>  	}
>
> +	/* Checksum Offload */
> +	switch (cookie->ol_flags & PKT_TX_L4_MASK) {
> +	case PKT_TX_UDP_CKSUM:
> +		hdr->csum_start = cookie->l2_len + cookie->l3_len;
> +		hdr->csum_offset = 6;
> +		hdr->flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
> +		break;
> +
> +	case PKT_TX_TCP_CKSUM:
> +		hdr->csum_start = cookie->l2_len + cookie->l3_len;
> +		hdr->csum_offset = 16;
> +		hdr->flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
> +		break;
> +
> +	default:
> +		hdr->csum_start = 0;
> +		hdr->csum_offset = 0;
> +		hdr->flags = 0;
> +		break;
> +	}
> +
> +	hdr->gso_type = 0;
> +	hdr->gso_size = 0;
> +	hdr->hdr_len = 0;

In he case we don't use any offload, have you measured the performance 
regression
with current code when using a dedicated descriptor for the header?
I haven't tested you series, but I would think it is more than 15% for
64 bytes packets based on my trials to use a single descriptor.

Indeed, without your series, when using a dedicated desc for the header,
the header is not accessed in the virtio transmit path. It is zeroed at
init time.

Could we keep the same behaviour when offloading features aren't
negotiated?

Thanks,
Maxime