[dpdk-dev] [PATCH 0/4] net/virtio: Tx path selection and offload improvements

[dpdk-dev] [PATCH 0/4] net/virtio: Tx path selection and offload improvements

[Maxime Coquelin]

This series addresses a problem seen when running benchmars,
where we see a big difference in Tx performance depending on
whether Rx mergeable is enabled or not.
With patch 1, Tx simple path is selected even when Rx-mrg is
negotiated.

Digging a bit further, I found that Tx simple path could be
selected even when offload features had been negotiated.
With patch 2, guest does not try to negotiate Tx offload
features that haven't been selected by the application.
It means that without restarting the guest, we can now switch
from Tx simple path when application do no request offloads,
to Tx standard path when the application request offloads.
Note that to do so, one must stop the port first [0].
Another advantage of doing this than using Tx simple path
when guest application does not use offload features, is
that on host side, the virtio net header parsing is skipped
in dequeue function.

Patch 3 fixes an issue with Rx offload, as simple path was
be used if application requested TCP LRO only.

Finally, patch 4 aims at improving the offload feature checks
in the standard paths.

Below are benchmarks results when offloads are enabled at
device level.

Rx-mrg=off benchmarks:
+------------+-------+-------------+-------------+----------+
|    Run     |  PVP  | Guest->Host | Host->Guest | Loopback |
+------------+-------+-------------+-------------+----------+
| v18.05     | 14.47 |       17.02 |       17.57 |    13.15 |
| + series   | 14.88 |       19.64 |       17.63 |    13.11 |
+------------+-------+-------------+-------------+----------+

Rx-mrg=on benchmarks:
+------------+-------+-------------+-------------+----------+
|    Run     |  PVP  | Guest->Host | Host->Guest | Loopback |
+------------+-------+-------------+-------------+----------+
| v18.05     |  9.53 |       13.80 |       16.70 |    13.11 |
| + series   | 12.62 |       19.69 |       16.70 |    13.11 |
+------------+-------+-------------+-------------+----------+

[0]:
testpmd> port start 0
EAL: Error disabling MSI-X interrupts for fd 17
set_rxtx_funcs(): virtio: using mergeable buffer Rx path on port 0
set_rxtx_funcs(): virtio: using simple Tx path on port 0
Port 0: 52:54:00:58:D7:01
Checking link statuses...
Done
testpmd> show port 0 tx_offload capabilities 
Tx Offloading Capabilities of port 0 :
  Per Queue :
  Per Port  : VLAN_INSERT UDP_CKSUM TCP_CKSUM TCP_TSO MULTI_SEGS
         
testpmd> show port 0 tx_offload configuration
Tx Offloading Configuration of port 0 :
  Port :
  Queue[ 0] :

testpmd> port stop 0
Stopping ports...
Checking link statuses...
Done                
testpmd> csum set tcp hw 0
Parse tunnel is off
IP checksum offload is sw
UDP checksum offload is sw
TCP checksum offload is hw
SCTP checksum offload is sw
Outer-Ip checksum offload is sw
testpmd> port start 0
Configuring Port 0 (socket 0)
EAL: Error disabling MSI-X interrupts for fd 17
set_rxtx_funcs(): virtio: using mergeable buffer Rx path on port 0
set_rxtx_funcs(): virtio: using standard Tx path on port 0
Port 0: 52:54:00:58:D7:01
Checking link statuses...
Done                                         
testpmd> show port 0 tx_offload configuration
Tx Offloading Configuration of port 0 :
  Port : TCP_CKSUM
  Queue[ 0] :


Maxime Coquelin (4):
  net/virtio: use simple path for Tx even if Rx mergeable
  net/vhost: improve Tx path selection
  net/virtio: don't use simple Rx path if TCP LRO requested
  net/virtio: improve offload check performance

 drivers/net/virtio/virtio_ethdev.c | 43 ++++++++++++++++++++++++++++++++++----
 drivers/net/virtio/virtio_ethdev.h |  3 ---
 drivers/net/virtio/virtio_pci.h    |  2 ++
 drivers/net/virtio/virtio_rxtx.c   | 29 ++++++-------------------
 4 files changed, 47 insertions(+), 30 deletions(-)

-- 
2.14.3

[dpdk-dev] [PATCH 1/4] net/virtio: use simple path for Tx even if Rx mergeable

[Maxime Coquelin]

Having Rx mergeable buffers feature enabled should not be
a reason to not use Tx simple path.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_ethdev.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 5833dad73..e68e9d067 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1879,7 +1879,6 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 #endif
 	if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
 		hw->use_simple_rx = 0;
-		hw->use_simple_tx = 0;
 	}
 
 	if (rx_offloads & (DEV_RX_OFFLOAD_UDP_CKSUM |
-- 
2.14.3

[dpdk-dev] [PATCH 2/4] net/vhost: improve Tx path selection

[Maxime Coquelin]

This patch improves the Tx path selection depending on
whether the application request for offloads, and on whether
offload features have been negotiated.

When the application doesn't request for Tx offload features,
the corresponding features bits aren't negotiated.

When Tx offload virtio features have been negotiated, ensure
the simple Tx path isn't selected.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_ethdev.c | 20 ++++++++++++++++++--
 drivers/net/virtio/virtio_ethdev.h |  3 ---
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index e68e9d067..5730620ed 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1798,8 +1798,10 @@ static int
 virtio_dev_configure(struct rte_eth_dev *dev)
 {
 	const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
+	const struct rte_eth_txmode *txmode = &dev->data->dev_conf.txmode;
 	struct virtio_hw *hw = dev->data->dev_private;
 	uint64_t rx_offloads = rxmode->offloads;
+	uint64_t tx_offloads = txmode->offloads;
 	uint64_t req_features;
 	int ret;
 
@@ -1821,6 +1823,15 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 			(1ULL << VIRTIO_NET_F_GUEST_TSO4) |
 			(1ULL << VIRTIO_NET_F_GUEST_TSO6);
 
+	if (tx_offloads & (DEV_TX_OFFLOAD_TCP_CKSUM |
+				DEV_TX_OFFLOAD_UDP_CKSUM))
+		req_features |= (1ULL << VIRTIO_NET_F_CSUM);
+
+	if (tx_offloads & DEV_TX_OFFLOAD_TCP_TSO)
+		req_features |=
+			(1ULL << VIRTIO_NET_F_HOST_TSO4) |
+			(1ULL << VIRTIO_NET_F_HOST_TSO6);
+
 	/* if request features changed, reinit the device */
 	if (req_features != hw->req_guest_features) {
 		ret = virtio_init_device(dev, req_features);
@@ -1885,6 +1896,11 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 			   DEV_RX_OFFLOAD_TCP_CKSUM))
 		hw->use_simple_rx = 0;
 
+	if (tx_offloads & (DEV_TX_OFFLOAD_TCP_CKSUM |
+				DEV_TX_OFFLOAD_UDP_CKSUM |
+				DEV_TX_OFFLOAD_TCP_TSO))
+		hw->use_simple_tx = 0;
+
 	return 0;
 }
 
@@ -2138,14 +2154,14 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 
 	dev_info->tx_offload_capa = DEV_TX_OFFLOAD_MULTI_SEGS |
 				    DEV_TX_OFFLOAD_VLAN_INSERT;
-	if (hw->guest_features & (1ULL << VIRTIO_NET_F_CSUM)) {
+	if (host_features & (1ULL << VIRTIO_NET_F_CSUM)) {
 		dev_info->tx_offload_capa |=
 			DEV_TX_OFFLOAD_UDP_CKSUM |
 			DEV_TX_OFFLOAD_TCP_CKSUM;
 	}
 	tso_mask = (1ULL << VIRTIO_NET_F_HOST_TSO4) |
 		(1ULL << VIRTIO_NET_F_HOST_TSO6);
-	if ((hw->guest_features & tso_mask) == tso_mask)
+	if ((host_features & tso_mask) == tso_mask)
 		dev_info->tx_offload_capa |= DEV_TX_OFFLOAD_TCP_TSO;
 }
 
diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
index bb40064ea..b603665c7 100644
--- a/drivers/net/virtio/virtio_ethdev.h
+++ b/drivers/net/virtio/virtio_ethdev.h
@@ -28,9 +28,6 @@
 	 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_HOST_TSO4	  |	\
-	 1u << VIRTIO_NET_F_HOST_TSO6	  |	\
 	 1u << VIRTIO_NET_F_MRG_RXBUF	  |	\
 	 1u << VIRTIO_NET_F_MTU	| \
 	 1ULL << VIRTIO_NET_F_GUEST_ANNOUNCE |	\
-- 
2.14.3

Re: [dpdk-dev] [PATCH 2/4] net/vhost: improve Tx path selection

[Tiwei Bie]

On Fri, Jun 01, 2018 at 02:47:56PM +0200, Maxime Coquelin wrote:
> This patch improves the Tx path selection depending on
> whether the application request for offloads, and on whether
> offload features have been negotiated.
> 
> When the application doesn't request for Tx offload features,
> the corresponding features bits aren't negotiated.
> 
> When Tx offload virtio features have been negotiated, ensure
> the simple Tx path isn't selected.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c | 20 ++++++++++++++++++--
>  drivers/net/virtio/virtio_ethdev.h |  3 ---
>  2 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index e68e9d067..5730620ed 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1798,8 +1798,10 @@ static int
>  virtio_dev_configure(struct rte_eth_dev *dev)
>  {
>  	const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
> +	const struct rte_eth_txmode *txmode = &dev->data->dev_conf.txmode;
>  	struct virtio_hw *hw = dev->data->dev_private;
>  	uint64_t rx_offloads = rxmode->offloads;
> +	uint64_t tx_offloads = txmode->offloads;
>  	uint64_t req_features;
>  	int ret;
>  
> @@ -1821,6 +1823,15 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>  			(1ULL << VIRTIO_NET_F_GUEST_TSO4) |
>  			(1ULL << VIRTIO_NET_F_GUEST_TSO6);
>  
> +	if (tx_offloads & (DEV_TX_OFFLOAD_TCP_CKSUM |
> +				DEV_TX_OFFLOAD_UDP_CKSUM))

I think it's better to keep DEV_TX_OFFLOAD_TCP/UDP_CKSUM
aligned, something like:

	if (tx_offloads & (DEV_TX_OFFLOAD_TCP_CKSUM |
			   DEV_TX_OFFLOAD_UDP_CKSUM))

> +		req_features |= (1ULL << VIRTIO_NET_F_CSUM);
> +
> +	if (tx_offloads & DEV_TX_OFFLOAD_TCP_TSO)
> +		req_features |=
> +			(1ULL << VIRTIO_NET_F_HOST_TSO4) |
> +			(1ULL << VIRTIO_NET_F_HOST_TSO6);
> +
>  	/* if request features changed, reinit the device */
>  	if (req_features != hw->req_guest_features) {
>  		ret = virtio_init_device(dev, req_features);
> @@ -1885,6 +1896,11 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>  			   DEV_RX_OFFLOAD_TCP_CKSUM))
>  		hw->use_simple_rx = 0;
>  
> +	if (tx_offloads & (DEV_TX_OFFLOAD_TCP_CKSUM |
> +				DEV_TX_OFFLOAD_UDP_CKSUM |
> +				DEV_TX_OFFLOAD_TCP_TSO))

Ditto. I think it's better to keep them aligned,
something like:

	if (tx_offloads & (DEV_TX_OFFLOAD_TCP_CKSUM |
			   DEV_TX_OFFLOAD_UDP_CKSUM |
			   DEV_TX_OFFLOAD_TCP_TSO))

Besides, we also need to consider not using simple Tx
when DEV_TX_OFFLOAD_VLAN_INSERT is requested.

Best regards,
Tiwei Bie

> +		hw->use_simple_tx = 0;
> +
>  	return 0;
>  }
>  
> @@ -2138,14 +2154,14 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
>  
>  	dev_info->tx_offload_capa = DEV_TX_OFFLOAD_MULTI_SEGS |
>  				    DEV_TX_OFFLOAD_VLAN_INSERT;
> -	if (hw->guest_features & (1ULL << VIRTIO_NET_F_CSUM)) {
> +	if (host_features & (1ULL << VIRTIO_NET_F_CSUM)) {
>  		dev_info->tx_offload_capa |=
>  			DEV_TX_OFFLOAD_UDP_CKSUM |
>  			DEV_TX_OFFLOAD_TCP_CKSUM;
>  	}
>  	tso_mask = (1ULL << VIRTIO_NET_F_HOST_TSO4) |
>  		(1ULL << VIRTIO_NET_F_HOST_TSO6);
> -	if ((hw->guest_features & tso_mask) == tso_mask)
> +	if ((host_features & tso_mask) == tso_mask)
>  		dev_info->tx_offload_capa |= DEV_TX_OFFLOAD_TCP_TSO;
>  }
>  
> diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
> index bb40064ea..b603665c7 100644
> --- a/drivers/net/virtio/virtio_ethdev.h
> +++ b/drivers/net/virtio/virtio_ethdev.h
> @@ -28,9 +28,6 @@
>  	 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_HOST_TSO4	  |	\
> -	 1u << VIRTIO_NET_F_HOST_TSO6	  |	\
>  	 1u << VIRTIO_NET_F_MRG_RXBUF	  |	\
>  	 1u << VIRTIO_NET_F_MTU	| \
>  	 1ULL << VIRTIO_NET_F_GUEST_ANNOUNCE |	\
> -- 
> 2.14.3
> 

[dpdk-dev] [PATCH 3/4] net/virtio: don't use simple Rx path if TCP LRO requested

[Maxime Coquelin]

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_ethdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 5730620ed..d481b282e 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1893,7 +1893,8 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 	}
 
 	if (rx_offloads & (DEV_RX_OFFLOAD_UDP_CKSUM |
-			   DEV_RX_OFFLOAD_TCP_CKSUM))
+			   DEV_RX_OFFLOAD_TCP_CKSUM |
+			   DEV_RX_OFFLOAD_TCP_LRO))
 		hw->use_simple_rx = 0;
 
 	if (tx_offloads & (DEV_TX_OFFLOAD_TCP_CKSUM |
-- 
2.14.3

Re: [dpdk-dev] [PATCH 3/4] net/virtio: don't use simple Rx path if TCP LRO requested

[Tiwei Bie]

On Fri, Jun 01, 2018 at 02:47:57PM +0200, Maxime Coquelin wrote:
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 5730620ed..d481b282e 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1893,7 +1893,8 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>  	}
>  
>  	if (rx_offloads & (DEV_RX_OFFLOAD_UDP_CKSUM |
> -			   DEV_RX_OFFLOAD_TCP_CKSUM))
> +			   DEV_RX_OFFLOAD_TCP_CKSUM |
> +			   DEV_RX_OFFLOAD_TCP_LRO))

Maybe we also need to consider not using simple Rx
when DEV_RX_OFFLOAD_VLAN_STRIP is requested.

Best regards,
Tiwei Bie

>  		hw->use_simple_rx = 0;
>  
>  	if (tx_offloads & (DEV_TX_OFFLOAD_TCP_CKSUM |
> -- 
> 2.14.3
> 

Re: [dpdk-dev] [PATCH 3/4] net/virtio: don't use simple Rx path if TCP LRO requested

[Maxime Coquelin]

On 06/04/2018 01:59 PM, Tiwei Bie wrote:
> On Fri, Jun 01, 2018 at 02:47:57PM +0200, Maxime Coquelin wrote:
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>   drivers/net/virtio/virtio_ethdev.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
>> index 5730620ed..d481b282e 100644
>> --- a/drivers/net/virtio/virtio_ethdev.c
>> +++ b/drivers/net/virtio/virtio_ethdev.c
>> @@ -1893,7 +1893,8 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>>   	}
>>   
>>   	if (rx_offloads & (DEV_RX_OFFLOAD_UDP_CKSUM |
>> -			   DEV_RX_OFFLOAD_TCP_CKSUM))
>> +			   DEV_RX_OFFLOAD_TCP_CKSUM |
>> +			   DEV_RX_OFFLOAD_TCP_LRO))
> 
> Maybe we also need to consider not using simple Rx
> when DEV_RX_OFFLOAD_VLAN_STRIP is requested.

I think that makes sense.

Thanks,
Maxime

> Best regards,
> Tiwei Bie
> 
>>   		hw->use_simple_rx = 0;
>>   
>>   	if (tx_offloads & (DEV_TX_OFFLOAD_TCP_CKSUM |
>> -- 
>> 2.14.3
>>

Re: [dpdk-dev] [PATCH 3/4] net/virtio: don't use simple Rx path if TCP LRO requested

[Maxime Coquelin]

On 06/04/2018 04:32 PM, Maxime Coquelin wrote:
> 
> 
> On 06/04/2018 01:59 PM, Tiwei Bie wrote:
>> On Fri, Jun 01, 2018 at 02:47:57PM +0200, Maxime Coquelin wrote:
>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> ---
>>>   drivers/net/virtio/virtio_ethdev.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/virtio/virtio_ethdev.c 
>>> b/drivers/net/virtio/virtio_ethdev.c
>>> index 5730620ed..d481b282e 100644
>>> --- a/drivers/net/virtio/virtio_ethdev.c
>>> +++ b/drivers/net/virtio/virtio_ethdev.c
>>> @@ -1893,7 +1893,8 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>>>       }
>>>       if (rx_offloads & (DEV_RX_OFFLOAD_UDP_CKSUM |
>>> -               DEV_RX_OFFLOAD_TCP_CKSUM))
>>> +               DEV_RX_OFFLOAD_TCP_CKSUM |
>>> +               DEV_RX_OFFLOAD_TCP_LRO))
>>
>> Maybe we also need to consider not using simple Rx
>> when DEV_RX_OFFLOAD_VLAN_STRIP is requested.
> 
> I think that makes sense.

And also the same for Tx when VLAN_INSERT is requested

> Thanks,
> Maxime
> 
>> Best regards,
>> Tiwei Bie
>>
>>>           hw->use_simple_rx = 0;
>>>       if (tx_offloads & (DEV_TX_OFFLOAD_TCP_CKSUM |
>>> -- 
>>> 2.14.3
>>>

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

[Maxime Coquelin]

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
+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
+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;
 	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);
+
 	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;
 }
 
-static inline int
-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);
-}
-
 #define VIRTIO_MBUF_BURST_SZ 64
 #define DESC_PER_CACHELINE (RTE_CACHE_LINE_SIZE / sizeof(struct vring_desc))
 uint16_t
@@ -709,7 +695,6 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 	int error;
 	uint32_t i, nb_enqueued;
 	uint32_t hdr_size;
-	int offload;
 	struct virtio_net_hdr *hdr;
 
 	nb_rx = 0;
@@ -731,7 +716,6 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 
 	nb_enqueued = 0;
 	hdr_size = hw->vtnet_hdr_size;
-	offload = rx_offload_enabled(hw);
 
 	for (i = 0; i < num ; i++) {
 		rxm = rcv_pkts[i];
@@ -760,7 +744,7 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 		if (hw->vlan_strip)
 			rte_vlan_strip(rxm);
 
-		if (offload && virtio_rx_offload(rxm, hdr) < 0) {
+		if (hw->has_rx_offload && virtio_rx_offload(rxm, hdr) < 0) {
 			virtio_discard_rxbuf(vq, rxm);
 			rxvq->stats.errors++;
 			continue;
@@ -825,7 +809,6 @@ virtio_recv_mergeable_pkts(void *rx_queue,
 	uint16_t extra_idx;
 	uint32_t seg_res;
 	uint32_t hdr_size;
-	int offload;
 
 	nb_rx = 0;
 	if (unlikely(hw->started == 0))
@@ -843,7 +826,6 @@ virtio_recv_mergeable_pkts(void *rx_queue,
 	extra_idx = 0;
 	seg_res = 0;
 	hdr_size = hw->vtnet_hdr_size;
-	offload = rx_offload_enabled(hw);
 
 	while (i < nb_used) {
 		struct virtio_net_hdr_mrg_rxbuf *header;
@@ -888,7 +870,8 @@ virtio_recv_mergeable_pkts(void *rx_queue,
 		rx_pkts[nb_rx] = rxm;
 		prev = rxm;
 
-		if (offload && virtio_rx_offload(rxm, &header->hdr) < 0) {
+		if (hw->has_rx_offload &&
+				virtio_rx_offload(rxm, &header->hdr) < 0) {
 			virtio_discard_rxbuf(vq, rxm);
 			rxvq->stats.errors++;
 			continue;
-- 
2.14.3

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

[Tiwei Bie]

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.

> +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;

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.

> +
>  	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

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

[Maxime Coquelin]

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
> 

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

[Tiwei Bie]

On Mon, Jun 04, 2018 at 04:29:56PM +0200, Maxime Coquelin wrote:
> On 06/04/2018 01:55 PM, Tiwei Bie wrote:
> > On Fri, Jun 01, 2018 at 02:47:58PM +0200, Maxime Coquelin wrote:
[...]
> > > @@ -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.

Hmm.. Somehow I treated 'and' as 'or'..

I have another question. When 'can_push' is false
and 'vq->hw->has_tx_offload' is true, there will
be a chance that virtio net hdr won't be zeroed
when ol_flags doesn't specify any Tx offloads.

Best regards,
Tiwei Bie

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

[Maxime Coquelin]

On 06/05/2018 05:10 AM, Tiwei Bie wrote:
> On Mon, Jun 04, 2018 at 04:29:56PM +0200, Maxime Coquelin wrote:
>> On 06/04/2018 01:55 PM, Tiwei Bie wrote:
>>> On Fri, Jun 01, 2018 at 02:47:58PM +0200, Maxime Coquelin wrote:
> [...]
>>>> @@ -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.
> 
> Hmm.. Somehow I treated 'and' as 'or'..
> 
> I have another question. When 'can_push' is false
> and 'vq->hw->has_tx_offload' is true, there will
> be a chance that virtio net hdr won't be zeroed
> when ol_flags doesn't specify any Tx offloads.

Right, good catch.
It may be better to remove this small optimization.
Indeed, with the series, if the application does not enable offloads,
then the Virtio features are re-negotiated with the offload features.

Thanks,
Maxime

> Best regards,
> Tiwei Bie
> 

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

[Tiwei Bie]

On Tue, Jun 05, 2018 at 11:43:11AM +0200, Maxime Coquelin wrote:
> On 06/05/2018 05:10 AM, Tiwei Bie wrote:
> > On Mon, Jun 04, 2018 at 04:29:56PM +0200, Maxime Coquelin wrote:
> > > On 06/04/2018 01:55 PM, Tiwei Bie wrote:
> > > > On Fri, Jun 01, 2018 at 02:47:58PM +0200, Maxime Coquelin wrote:
> > [...]
> > > > > @@ -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.
> > 
> > Hmm.. Somehow I treated 'and' as 'or'..
> > 
> > I have another question. When 'can_push' is false
> > and 'vq->hw->has_tx_offload' is true, there will
> > be a chance that virtio net hdr won't be zeroed
> > when ol_flags doesn't specify any Tx offloads.
> 
> Right, good catch.
> It may be better to remove this small optimization.
> Indeed, with the series, if the application does not enable offloads,
> then the Virtio features are re-negotiated with the offload features.

Yeah. It's a good idea to disable the features when
the corresponding Tx offloads aren't requested by
the applications! I like it!

This issue happens for the mbufs whose ol_flags
doesn't specify Tx offloads when applications
enable Tx offloads and can_push is false. I think
when applications enable Tx offloads, although
most packets to be sent will have Tx offloads
specified in their ol_flags, it's still possible
that some packets don't have Tx offloads specified
in their ol_flags.

Best regards,
Tiwei Bie

> 
> Thanks,
> Maxime
> 
> > Best regards,
> > Tiwei Bie
> > 

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

[Maxime Coquelin]

On 06/05/2018 01:20 PM, Tiwei Bie wrote:
> On Tue, Jun 05, 2018 at 11:43:11AM +0200, Maxime Coquelin wrote:
>> On 06/05/2018 05:10 AM, Tiwei Bie wrote:
>>> On Mon, Jun 04, 2018 at 04:29:56PM +0200, Maxime Coquelin wrote:
>>>> On 06/04/2018 01:55 PM, Tiwei Bie wrote:
>>>>> On Fri, Jun 01, 2018 at 02:47:58PM +0200, Maxime Coquelin wrote:
>>> [...]
>>>>>> @@ -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.
>>>
>>> Hmm.. Somehow I treated 'and' as 'or'..
>>>
>>> I have another question. When 'can_push' is false
>>> and 'vq->hw->has_tx_offload' is true, there will
>>> be a chance that virtio net hdr won't be zeroed
>>> when ol_flags doesn't specify any Tx offloads.
>>
>> Right, good catch.
>> It may be better to remove this small optimization.
>> Indeed, with the series, if the application does not enable offloads,
>> then the Virtio features are re-negotiated with the offload features.
> 
> Yeah. It's a good idea to disable the features when
> the corresponding Tx offloads aren't requested by
> the applications! I like it!
> 
> This issue happens for the mbufs whose ol_flags
> doesn't specify Tx offloads when applications
> enable Tx offloads and can_push is false. I think
> when applications enable Tx offloads, although
> most packets to be sent will have Tx offloads
> specified in their ol_flags, it's still possible
> that some packets don't have Tx offloads specified
> in their ol_flags.

Reading again my reply, I think it wasn't clear enough,
let me rephrase it.

My proposal is to keep disabling the features if the corresponding
Tx offloads aren't negotiated by the application, but just to remove the
check on mbuf's ol_flags, so:
offload = vq->hw->has_tx_offload;

Doing that, we retrieve the old behaviour, i.e. if Virtio features are
negotiated but no ol_flags set, virtio-net header will be cleared.

Maxime
> Best regards,
> Tiwei Bie
> 
>>
>> Thanks,
>> Maxime
>>
>>> Best regards,
>>> Tiwei Bie
>>>

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

[Tiwei Bie]

On Tue, Jun 05, 2018 at 01:58:00PM +0200, Maxime Coquelin wrote:
> On 06/05/2018 01:20 PM, Tiwei Bie wrote:
> > On Tue, Jun 05, 2018 at 11:43:11AM +0200, Maxime Coquelin wrote:
> > > On 06/05/2018 05:10 AM, Tiwei Bie wrote:
> > > > On Mon, Jun 04, 2018 at 04:29:56PM +0200, Maxime Coquelin wrote:
> > > > > On 06/04/2018 01:55 PM, Tiwei Bie wrote:
> > > > > > On Fri, Jun 01, 2018 at 02:47:58PM +0200, Maxime Coquelin wrote:
> > > > [...]
> > > > > > > @@ -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.
> > > > 
> > > > Hmm.. Somehow I treated 'and' as 'or'..
> > > > 
> > > > I have another question. When 'can_push' is false
> > > > and 'vq->hw->has_tx_offload' is true, there will
> > > > be a chance that virtio net hdr won't be zeroed
> > > > when ol_flags doesn't specify any Tx offloads.
> > > 
> > > Right, good catch.
> > > It may be better to remove this small optimization.
> > > Indeed, with the series, if the application does not enable offloads,
> > > then the Virtio features are re-negotiated with the offload features.
> > 
> > Yeah. It's a good idea to disable the features when
> > the corresponding Tx offloads aren't requested by
> > the applications! I like it!
> > 
> > This issue happens for the mbufs whose ol_flags
> > doesn't specify Tx offloads when applications
> > enable Tx offloads and can_push is false. I think
> > when applications enable Tx offloads, although
> > most packets to be sent will have Tx offloads
> > specified in their ol_flags, it's still possible
> > that some packets don't have Tx offloads specified
> > in their ol_flags.
> 
> Reading again my reply, I think it wasn't clear enough,
> let me rephrase it.
> 
> My proposal is to keep disabling the features if the corresponding
> Tx offloads aren't negotiated by the application, but just to remove the
> check on mbuf's ol_flags, so:
> offload = vq->hw->has_tx_offload;
> 
> Doing that, we retrieve the old behaviour, i.e. if Virtio features are
> negotiated but no ol_flags set, virtio-net header will be cleared.

It sounds good! Thanks!

Best regards,
Tiwei Bie

Re: [dpdk-dev] [PATCH 0/4] net/virtio: Tx path selection and offload improvements

[Maxime Coquelin]

On 06/01/2018 02:47 PM, Maxime Coquelin wrote:
> Below are benchmarks results when offloads are enabled at
"When *no* offload" should be read here...

> device level.
> 
> Rx-mrg=off benchmarks:
> +------------+-------+-------------+-------------+----------+
> |    Run     |  PVP  | Guest->Host | Host->Guest | Loopback |
> +------------+-------+-------------+-------------+----------+
> | v18.05     | 14.47 |       17.02 |       17.57 |    13.15 |
> | + series   | 14.88 |       19.64 |       17.63 |    13.11 |
> +------------+-------+-------------+-------------+----------+
> 
> Rx-mrg=on benchmarks:
> +------------+-------+-------------+-------------+----------+
> |    Run     |  PVP  | Guest->Host | Host->Guest | Loopback |
> +------------+-------+-------------+-------------+----------+
> | v18.05     |  9.53 |       13.80 |       16.70 |    13.11 |
> | + series   | 12.62 |       19.69 |       16.70 |    13.11 |
> +------------+-------+-------------+-------------+----------+