Re: [dpdk-dev] [PATCH 4/4] net/virtio: improve offload check performance

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

On 06/04/2018 01:55 PM, Tiwei Bie wrote:
> On Fri, Jun 01, 2018 at 02:47:58PM +0200, Maxime Coquelin wrote:
>> Instead of checking the multiple Virtio features bits for
>> every packet, let's do the check once at configure time and
>> store it in virtio_hw struct.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>   drivers/net/virtio/virtio_ethdev.c | 19 +++++++++++++++++++
>>   drivers/net/virtio/virtio_pci.h    |  2 ++
>>   drivers/net/virtio/virtio_rxtx.c   | 29 ++++++-----------------------
>>   3 files changed, 27 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
>> index d481b282e..981e0994a 100644
>> --- a/drivers/net/virtio/virtio_ethdev.c
>> +++ b/drivers/net/virtio/virtio_ethdev.c
>> @@ -1790,6 +1790,22 @@ rte_virtio_pmd_init(void)
>>   	rte_pci_register(&rte_virtio_pmd);
>>   }
>>   
>> +static inline int
> 
> Maybe it's better to return bool.

Agree, I'll do that.
I just moved code from virtio_rxtx.c without change, but it is indeed
better to return bool.

>> +rx_offload_enabled(struct virtio_hw *hw)
>> +{
>> +	return vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_CSUM) ||
>> +		vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO4) ||
>> +		vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO6);
>> +}
>> +
>> +static inline int
> 
> Ditto.
> 
>> +tx_offload_enabled(struct virtio_hw *hw)
>> +{
>> +	return vtpci_with_feature(hw, VIRTIO_NET_F_CSUM) ||
>> +		vtpci_with_feature(hw, VIRTIO_NET_F_HOST_TSO4) ||
>> +		vtpci_with_feature(hw, VIRTIO_NET_F_HOST_TSO6);
>> +}
>> +
>>   /*
>>    * Configure virtio device
>>    * It returns 0 on success.
>> @@ -1869,6 +1885,9 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>>   		return -ENOTSUP;
>>   	}
>>   
>> +	hw->has_tx_offload = !!tx_offload_enabled(hw);
>> +	hw->has_rx_offload = !!rx_offload_enabled(hw);
>> +
>>   	if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
>>   		/* Enable vector (0) for Link State Intrerrupt */
>>   		if (VTPCI_OPS(hw)->set_config_irq(hw, 0) ==
>> diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
>> index a28ba8339..e0bb871f2 100644
>> --- a/drivers/net/virtio/virtio_pci.h
>> +++ b/drivers/net/virtio/virtio_pci.h
>> @@ -233,6 +233,8 @@ struct virtio_hw {
>>   	uint8_t     modern;
>>   	uint8_t     use_simple_rx;
>>   	uint8_t     use_simple_tx;
>> +	uint8_t		has_tx_offload;
>> +	uint8_t		has_rx_offload;

Acked.

> 
> I think it's better to use spaces, instead of
> 4 spaces width tabs after uint8_t.
> 
>>   	uint16_t    port_id;
>>   	uint8_t     mac_addr[ETHER_ADDR_LEN];
>>   	uint32_t    notify_off_multiplier;
>> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
>> index 92fab2174..3f113a118 100644
>> --- a/drivers/net/virtio/virtio_rxtx.c
>> +++ b/drivers/net/virtio/virtio_rxtx.c
>> @@ -225,13 +225,6 @@ virtio_tso_fix_cksum(struct rte_mbuf *m)
>>   	}
>>   }
>>   
>> -static inline int
>> -tx_offload_enabled(struct virtio_hw *hw)
>> -{
>> -	return vtpci_with_feature(hw, VIRTIO_NET_F_CSUM) ||
>> -		vtpci_with_feature(hw, VIRTIO_NET_F_HOST_TSO4) ||
>> -		vtpci_with_feature(hw, VIRTIO_NET_F_HOST_TSO6);
>> -}
>>   
>>   /* avoid write operation when necessary, to lessen cache issues */
>>   #define ASSIGN_UNLESS_EQUAL(var, val) do {	\
>> @@ -253,13 +246,15 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct rte_mbuf *cookie,
>>   	struct virtio_net_hdr *hdr;
>>   	int offload;
>>   
>> -	offload = tx_offload_enabled(vq->hw);
>>   	head_idx = vq->vq_desc_head_idx;
>>   	idx = head_idx;
>>   	dxp = &vq->vq_descx[idx];
>>   	dxp->cookie = (void *)cookie;
>>   	dxp->ndescs = needed;
>>   
>> +	offload = vq->hw->has_tx_offload &&
>> +		(cookie->ol_flags & PKT_TX_OFFLOAD_MASK);
> 
> If features aren't negotiated, I think there is no need to
> check ol_flags and update the net header.

Isn't what the code is doing?
has_tx_offload will be false if none of the Tx offload features have
been negotiated, so ol_flags won't be checked in that case.

>> +
>>   	start_dp = vq->vq_ring.desc;
>>   
>>   	if (can_push) {
>> @@ -270,7 +265,6 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct rte_mbuf *cookie,
>>   		 * which is wrong. Below subtract restores correct pkt size.
>>   		 */
>>   		cookie->pkt_len -= head_size;
>> -		/* if offload disabled, it is not zeroed below, do it now */
>>   		if (offload == 0) {
>>   			ASSIGN_UNLESS_EQUAL(hdr->csum_start, 0);
>>   			ASSIGN_UNLESS_EQUAL(hdr->csum_offset, 0);
>> @@ -686,14 +680,6 @@ virtio_rx_offload(struct rte_mbuf *m, struct virtio_net_hdr *hdr)
>>   	return 0;
>>   }
> [...]
> 
> Best regards,
> Tiwei Bie
>