[v5,8/9] net/virtio: add in-order Rx/Tx into selection

Message ID	20180702135642.52577-9-yong.liu@intel.com (mailing list archive)
State	Accepted, archived
Delegated to:	Maxime Coquelin
Headers	show Return-Path: <dev-bounces@dpdk.org> From: Marvin Liu <yong.liu@intel.com> To: maxime.coquelin@redhat.com, tiwei.bie@intel.com Cc: zhihong.wang@intel.com, dev@dpdk.org, Marvin Liu <yong.liu@intel.com> Date: Mon, 2 Jul 2018 21:56:41 +0800 Message-Id: <20180702135642.52577-9-yong.liu@intel.com> In-Reply-To: <20180702135642.52577-1-yong.liu@intel.com> References: <20180702135642.52577-1-yong.liu@intel.com> Subject: [dpdk-dev] [PATCH v5 8/9] net/virtio: add in-order Rx/Tx into selection Precedence: list Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org>
Series	support in-order feature \| expand [v5,0/9] support in-order feature [v5,1/9] vhost: advertise support in-order feature [v5,2/9] net/virtio: add in-order feature bit definition [v5,3/9] net/virtio-user: add unsupported features mask [v5,4/9] net/virtio-user: add mrg-rxbuf and in-order vdev parameters [v5,5/9] net/virtio: free in-order descriptors before device start [v5,6/9] net/virtio: extract common part for in-order functions [v5,7/9] net/virtio: support in-order Rx and Tx [v5,8/9] net/virtio: add in-order Rx/Tx into selection [v5,9/9] net/virtio: advertise support in-order feature

Message ID

20180702135642.52577-9-yong.liu@intel.com (mailing list archive)

State

Accepted, archived

Delegated to:

Maxime Coquelin

Headers

From: Marvin Liu <yong.liu@intel.com>
To: maxime.coquelin@redhat.com,
	tiwei.bie@intel.com
Cc: zhihong.wang@intel.com,
	dev@dpdk.org,
	Marvin Liu <yong.liu@intel.com>
Date: Mon,  2 Jul 2018 21:56:41 +0800
Message-Id: <20180702135642.52577-9-yong.liu@intel.com>
In-Reply-To: <20180702135642.52577-1-yong.liu@intel.com>
References: <20180702135642.52577-1-yong.liu@intel.com>
Subject: [dpdk-dev] [PATCH v5 8/9] net/virtio: add in-order Rx/Tx into
	selection
Precedence: list
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

Series

support in-order feature | expand

Context	Check	Description
ci/checkpatch	success	coding style OK
ci/Intel-compilation	success	Compilation OK

Context

Check

Description

ci/checkpatch

success

coding style OK

ci/Intel-compilation

success

Compilation OK

Commit Message

Marvin Liu July 2, 2018, 1:56 p.m. UTC

After IN_ORDER Rx/Tx paths added, need to update Rx/Tx path selection
logic.

Rx path select logic: If IN_ORDER and merge-able are enabled will select
IN_ORDER Rx path. If IN_ORDER is enabled, Rx offload and merge-able are
disabled will select simple Rx path. Otherwise will select normal Rx
path.

Tx path select logic: If IN_ORDER is enabled will select IN_ORDER Tx
path. Otherwise will select default Tx path.

Signed-off-by: Marvin Liu <yong.liu@intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Comments

Maxime Coquelin July 2, 2018, 11:24 a.m. UTC | #1

On 07/02/2018 03:56 PM, Marvin Liu wrote:
> After IN_ORDER Rx/Tx paths added, need to update Rx/Tx path selection
> logic.
> 
> Rx path select logic: If IN_ORDER and merge-able are enabled will select
> IN_ORDER Rx path. If IN_ORDER is enabled, Rx offload and merge-able are
> disabled will select simple Rx path. Otherwise will select normal Rx
> path.
> 
> Tx path select logic: If IN_ORDER is enabled will select IN_ORDER Tx
> path. Otherwise will select default Tx path.
> 
> Signed-off-by: Marvin Liu <yong.liu@intel.com>
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> diff --git a/doc/guides/nics/virtio.rst b/doc/guides/nics/virtio.rst
> index 46e292c4d..7c099fb7c 100644
> --- a/doc/guides/nics/virtio.rst
> +++ b/doc/guides/nics/virtio.rst
> @@ -201,7 +201,7 @@ The packet transmission flow is:
>   Virtio PMD Rx/Tx Callbacks
>   --------------------------
>   
> -Virtio driver has 3 Rx callbacks and 2 Tx callbacks.
> +Virtio driver has 4 Rx callbacks and 3 Tx callbacks.
>   
>   Rx callbacks:
>   
> @@ -215,6 +215,9 @@ Rx callbacks:
>      Vector version without mergeable Rx buffer support, also fixes the available
>      ring indexes and uses vector instructions to optimize performance.
>   
> +#. ``virtio_recv_mergeable_pkts_inorder``:
> +   In-order version with mergeable Rx buffer support.
> +
>   Tx callbacks:
>   
>   #. ``virtio_xmit_pkts``:
> @@ -223,6 +226,8 @@ Tx callbacks:
>   #. ``virtio_xmit_pkts_simple``:
>      Vector version fixes the available ring indexes to optimize performance.
>   
> +#. ``virtio_xmit_pkts_inorder``:
> +   In-order version.
>   
>   By default, the non-vector callbacks are used:
>   
> @@ -254,6 +259,12 @@ Example of using the vector version of the virtio poll mode driver in
>   
>      testpmd -l 0-2 -n 4 -- -i --tx-offloads=0x0 --rxq=1 --txq=1 --nb-cores=1
>   
> +In-order callbacks only work on simulated virtio user vdev.
> +
> +*   For Rx: If mergeable Rx buffers is enabled and in-order is enabled then
> +    ``virtio_xmit_pkts_inorder`` is used.
> +
> +*   For Tx: If in-order is enabled then ``virtio_xmit_pkts_inorder`` is used.
>   
>   Interrupt mode
>   --------------
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index df50a571a..df7981ddb 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1320,6 +1320,11 @@ set_rxtx_funcs(struct rte_eth_dev *eth_dev)
>   		PMD_INIT_LOG(INFO, "virtio: using simple Rx path on port %u",
>   			eth_dev->data->port_id);
>   		eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
> +	} else if (hw->use_inorder_rx) {
> +		PMD_INIT_LOG(INFO,
> +			"virtio: using inorder mergeable buffer Rx path on port %u",
> +			eth_dev->data->port_id);
> +		eth_dev->rx_pkt_burst = &virtio_recv_mergeable_pkts_inorder;
>   	} else if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
>   		PMD_INIT_LOG(INFO,
>   			"virtio: using mergeable buffer Rx path on port %u",
> @@ -1335,6 +1340,10 @@ set_rxtx_funcs(struct rte_eth_dev *eth_dev)
>   		PMD_INIT_LOG(INFO, "virtio: using simple Tx path on port %u",
>   			eth_dev->data->port_id);
>   		eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
> +	} else if (hw->use_inorder_tx) {
> +		PMD_INIT_LOG(INFO, "virtio: using inorder Tx path on port %u",
> +			eth_dev->data->port_id);
> +		eth_dev->tx_pkt_burst = virtio_xmit_pkts_inorder;
>   	} else {
>   		PMD_INIT_LOG(INFO, "virtio: using standard Tx path on port %u",
>   			eth_dev->data->port_id);
> @@ -1874,20 +1883,27 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>   	hw->use_simple_rx = 1;
>   	hw->use_simple_tx = 1;
>   
> +	if (vtpci_with_feature(hw, VIRTIO_F_IN_ORDER)) {
> +		/* Simple Tx not compatible with in-order ring */
> +		hw->use_inorder_tx = 1;
> +		hw->use_simple_tx = 0;
> +		if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
> +			hw->use_inorder_rx = 1;
> +			hw->use_simple_rx = 0;
> +		} else {
> +			hw->use_inorder_rx = 0;
> +			if (rx_offloads & (DEV_RX_OFFLOAD_UDP_CKSUM |
> +					   DEV_RX_OFFLOAD_TCP_CKSUM))
> +				hw->use_simple_rx = 0;
It is applied, but I think this is still not good.

Simple Rx is set to 1 by default, so if IN_ORDER isn't negotiated,
and UDP/TCP_CSUM is enabled, simple Rx keeps being selected.

I'll fix it in my series that I'm doing on top.

Regards,
Maxime

> +		}
> +	}
> +
>   #if defined RTE_ARCH_ARM64 || defined RTE_ARCH_ARM
>   	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON)) {
>   		hw->use_simple_rx = 0;
>   		hw->use_simple_tx = 0;
>   	}
>   #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 |
> -			   DEV_RX_OFFLOAD_TCP_CKSUM))
> -		hw->use_simple_rx = 0;
>   
>   	return 0;
>   }
>

Maxime Coquelin July 2, 2018, 12:41 p.m. UTC | #2

On 07/02/2018 01:24 PM, Maxime Coquelin wrote:
> 
> 
> On 07/02/2018 03:56 PM, Marvin Liu wrote:
>> After IN_ORDER Rx/Tx paths added, need to update Rx/Tx path selection
>> logic.
>>
>> Rx path select logic: If IN_ORDER and merge-able are enabled will select
>> IN_ORDER Rx path. If IN_ORDER is enabled, Rx offload and merge-able are
>> disabled will select simple Rx path. Otherwise will select normal Rx
>> path.
>>
>> Tx path select logic: If IN_ORDER is enabled will select IN_ORDER Tx
>> path. Otherwise will select default Tx path.
>>
>> Signed-off-by: Marvin Liu <yong.liu@intel.com>
>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>
>> diff --git a/doc/guides/nics/virtio.rst b/doc/guides/nics/virtio.rst
>> index 46e292c4d..7c099fb7c 100644
>> --- a/doc/guides/nics/virtio.rst
>> +++ b/doc/guides/nics/virtio.rst
>> @@ -201,7 +201,7 @@ The packet transmission flow is:
>>   Virtio PMD Rx/Tx Callbacks
>>   --------------------------
>> -Virtio driver has 3 Rx callbacks and 2 Tx callbacks.
>> +Virtio driver has 4 Rx callbacks and 3 Tx callbacks.
>>   Rx callbacks:
>> @@ -215,6 +215,9 @@ Rx callbacks:
>>      Vector version without mergeable Rx buffer support, also fixes 
>> the available
>>      ring indexes and uses vector instructions to optimize performance.
>> +#. ``virtio_recv_mergeable_pkts_inorder``:
>> +   In-order version with mergeable Rx buffer support.
>> +
>>   Tx callbacks:
>>   #. ``virtio_xmit_pkts``:
>> @@ -223,6 +226,8 @@ Tx callbacks:
>>   #. ``virtio_xmit_pkts_simple``:
>>      Vector version fixes the available ring indexes to optimize 
>> performance.
>> +#. ``virtio_xmit_pkts_inorder``:
>> +   In-order version.
>>   By default, the non-vector callbacks are used:
>> @@ -254,6 +259,12 @@ Example of using the vector version of the virtio 
>> poll mode driver in
>>      testpmd -l 0-2 -n 4 -- -i --tx-offloads=0x0 --rxq=1 --txq=1 
>> --nb-cores=1
>> +In-order callbacks only work on simulated virtio user vdev.
>> +
>> +*   For Rx: If mergeable Rx buffers is enabled and in-order is 
>> enabled then
>> +    ``virtio_xmit_pkts_inorder`` is used.
>> +
>> +*   For Tx: If in-order is enabled then ``virtio_xmit_pkts_inorder`` 
>> is used.
>>   Interrupt mode
>>   --------------
>> diff --git a/drivers/net/virtio/virtio_ethdev.c 
>> b/drivers/net/virtio/virtio_ethdev.c
>> index df50a571a..df7981ddb 100644
>> --- a/drivers/net/virtio/virtio_ethdev.c
>> +++ b/drivers/net/virtio/virtio_ethdev.c
>> @@ -1320,6 +1320,11 @@ set_rxtx_funcs(struct rte_eth_dev *eth_dev)
>>           PMD_INIT_LOG(INFO, "virtio: using simple Rx path on port %u",
>>               eth_dev->data->port_id);
>>           eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
>> +    } else if (hw->use_inorder_rx) {
>> +        PMD_INIT_LOG(INFO,
>> +            "virtio: using inorder mergeable buffer Rx path on port %u",
>> +            eth_dev->data->port_id);
>> +        eth_dev->rx_pkt_burst = &virtio_recv_mergeable_pkts_inorder;
>>       } else if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
>>           PMD_INIT_LOG(INFO,
>>               "virtio: using mergeable buffer Rx path on port %u",
>> @@ -1335,6 +1340,10 @@ set_rxtx_funcs(struct rte_eth_dev *eth_dev)
>>           PMD_INIT_LOG(INFO, "virtio: using simple Tx path on port %u",
>>               eth_dev->data->port_id);
>>           eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
>> +    } else if (hw->use_inorder_tx) {
>> +        PMD_INIT_LOG(INFO, "virtio: using inorder Tx path on port %u",
>> +            eth_dev->data->port_id);
>> +        eth_dev->tx_pkt_burst = virtio_xmit_pkts_inorder;
>>       } else {
>>           PMD_INIT_LOG(INFO, "virtio: using standard Tx path on port %u",
>>               eth_dev->data->port_id);
>> @@ -1874,20 +1883,27 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>>       hw->use_simple_rx = 1;
>>       hw->use_simple_tx = 1;
>> +    if (vtpci_with_feature(hw, VIRTIO_F_IN_ORDER)) {
>> +        /* Simple Tx not compatible with in-order ring */
>> +        hw->use_inorder_tx = 1;
>> +        hw->use_simple_tx = 0;
>> +        if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
>> +            hw->use_inorder_rx = 1;
>> +            hw->use_simple_rx = 0;
>> +        } else {
>> +            hw->use_inorder_rx = 0;
>> +            if (rx_offloads & (DEV_RX_OFFLOAD_UDP_CKSUM |
>> +                       DEV_RX_OFFLOAD_TCP_CKSUM))
>> +                hw->use_simple_rx = 0;
> It is applied, but I think this is still not good.
> 
> Simple Rx is set to 1 by default, so if IN_ORDER isn't negotiated,
> and UDP/TCP_CSUM is enabled, simple Rx keeps being selected.
> 
> I'll fix it in my series that I'm doing on top.

Actually, after discussion with Ferruh, I fixed it directly in the patch.

Thanks,
Maxime

> Regards,
> Maxime
> 
>> +        }
>> +    }
>> +
>>   #if defined RTE_ARCH_ARM64 || defined RTE_ARCH_ARM
>>       if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON)) {
>>           hw->use_simple_rx = 0;
>>           hw->use_simple_tx = 0;
>>       }
>>   #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 |
>> -               DEV_RX_OFFLOAD_TCP_CKSUM))
>> -        hw->use_simple_rx = 0;
>>       return 0;
>>   }
>>

Maxime Coquelin July 2, 2018, 3:18 p.m. UTC | #3

On 07/02/2018 02:41 PM, Maxime Coquelin wrote:
> 
> 
> On 07/02/2018 01:24 PM, Maxime Coquelin wrote:
>>
>>
>> On 07/02/2018 03:56 PM, Marvin Liu wrote:
>>> After IN_ORDER Rx/Tx paths added, need to update Rx/Tx path selection
>>> logic.
>>>
>>> Rx path select logic: If IN_ORDER and merge-able are enabled will select
>>> IN_ORDER Rx path. If IN_ORDER is enabled, Rx offload and merge-able are
>>> disabled will select simple Rx path. Otherwise will select normal Rx
>>> path.
>>>
>>> Tx path select logic: If IN_ORDER is enabled will select IN_ORDER Tx
>>> path. Otherwise will select default Tx path.
>>>
>>> Signed-off-by: Marvin Liu <yong.liu@intel.com>
>>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>
>>> diff --git a/doc/guides/nics/virtio.rst b/doc/guides/nics/virtio.rst
>>> index 46e292c4d..7c099fb7c 100644
>>> --- a/doc/guides/nics/virtio.rst
>>> +++ b/doc/guides/nics/virtio.rst
>>> @@ -201,7 +201,7 @@ The packet transmission flow is:
>>>   Virtio PMD Rx/Tx Callbacks
>>>   --------------------------
>>> -Virtio driver has 3 Rx callbacks and 2 Tx callbacks.
>>> +Virtio driver has 4 Rx callbacks and 3 Tx callbacks.
>>>   Rx callbacks:
>>> @@ -215,6 +215,9 @@ Rx callbacks:
>>>      Vector version without mergeable Rx buffer support, also fixes 
>>> the available
>>>      ring indexes and uses vector instructions to optimize performance.
>>> +#. ``virtio_recv_mergeable_pkts_inorder``:
>>> +   In-order version with mergeable Rx buffer support.
>>> +
>>>   Tx callbacks:
>>>   #. ``virtio_xmit_pkts``:
>>> @@ -223,6 +226,8 @@ Tx callbacks:
>>>   #. ``virtio_xmit_pkts_simple``:
>>>      Vector version fixes the available ring indexes to optimize 
>>> performance.
>>> +#. ``virtio_xmit_pkts_inorder``:
>>> +   In-order version.
>>>   By default, the non-vector callbacks are used:
>>> @@ -254,6 +259,12 @@ Example of using the vector version of the 
>>> virtio poll mode driver in
>>>      testpmd -l 0-2 -n 4 -- -i --tx-offloads=0x0 --rxq=1 --txq=1 
>>> --nb-cores=1
>>> +In-order callbacks only work on simulated virtio user vdev.
>>> +
>>> +*   For Rx: If mergeable Rx buffers is enabled and in-order is 
>>> enabled then
>>> +    ``virtio_xmit_pkts_inorder`` is used.
>>> +
>>> +*   For Tx: If in-order is enabled then ``virtio_xmit_pkts_inorder`` 
>>> is used.
>>>   Interrupt mode
>>>   --------------
>>> diff --git a/drivers/net/virtio/virtio_ethdev.c 
>>> b/drivers/net/virtio/virtio_ethdev.c
>>> index df50a571a..df7981ddb 100644
>>> --- a/drivers/net/virtio/virtio_ethdev.c
>>> +++ b/drivers/net/virtio/virtio_ethdev.c
>>> @@ -1320,6 +1320,11 @@ set_rxtx_funcs(struct rte_eth_dev *eth_dev)
>>>           PMD_INIT_LOG(INFO, "virtio: using simple Rx path on port %u",
>>>               eth_dev->data->port_id);
>>>           eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
>>> +    } else if (hw->use_inorder_rx) {
>>> +        PMD_INIT_LOG(INFO,
>>> +            "virtio: using inorder mergeable buffer Rx path on port 
>>> %u",
>>> +            eth_dev->data->port_id);
>>> +        eth_dev->rx_pkt_burst = &virtio_recv_mergeable_pkts_inorder;
>>>       } else if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
>>>           PMD_INIT_LOG(INFO,
>>>               "virtio: using mergeable buffer Rx path on port %u",
>>> @@ -1335,6 +1340,10 @@ set_rxtx_funcs(struct rte_eth_dev *eth_dev)
>>>           PMD_INIT_LOG(INFO, "virtio: using simple Tx path on port %u",
>>>               eth_dev->data->port_id);
>>>           eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
>>> +    } else if (hw->use_inorder_tx) {
>>> +        PMD_INIT_LOG(INFO, "virtio: using inorder Tx path on port %u",
>>> +            eth_dev->data->port_id);
>>> +        eth_dev->tx_pkt_burst = virtio_xmit_pkts_inorder;
>>>       } else {
>>>           PMD_INIT_LOG(INFO, "virtio: using standard Tx path on port 
>>> %u",
>>>               eth_dev->data->port_id);
>>> @@ -1874,20 +1883,27 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>>>       hw->use_simple_rx = 1;
>>>       hw->use_simple_tx = 1;
>>> +    if (vtpci_with_feature(hw, VIRTIO_F_IN_ORDER)) {
>>> +        /* Simple Tx not compatible with in-order ring */
>>> +        hw->use_inorder_tx = 1;
>>> +        hw->use_simple_tx = 0;
>>> +        if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
>>> +            hw->use_inorder_rx = 1;
>>> +            hw->use_simple_rx = 0;
>>> +        } else {
>>> +            hw->use_inorder_rx = 0;
>>> +            if (rx_offloads & (DEV_RX_OFFLOAD_UDP_CKSUM |
>>> +                       DEV_RX_OFFLOAD_TCP_CKSUM))
>>> +                hw->use_simple_rx = 0;
>> It is applied, but I think this is still not good.
>>
>> Simple Rx is set to 1 by default, so if IN_ORDER isn't negotiated,
>> and UDP/TCP_CSUM is enabled, simple Rx keeps being selected.
>>
>> I'll fix it in my series that I'm doing on top.
> 
> Actually, after discussion with Ferruh, I fixed it directly in the patch.

Running some more tests, I noticed that it is still broken.
For example the case where host does not support IN_ORDER.
If mergeable buffer is negotiated, the simple path gets selected, which
is wrong.

I fixed that directly in the patch.

Regards,
Maxime
> Thanks,
> Maxime
> 
>> Regards,
>> Maxime
>>
>>> +        }
>>> +    }
>>> +
>>>   #if defined RTE_ARCH_ARM64 || defined RTE_ARCH_ARM
>>>       if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON)) {
>>>           hw->use_simple_rx = 0;
>>>           hw->use_simple_tx = 0;
>>>       }
>>>   #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 |
>>> -               DEV_RX_OFFLOAD_TCP_CKSUM))
>>> -        hw->use_simple_rx = 0;
>>>       return 0;
>>>   }
>>>

Marvin Liu July 3, 2018, 1:40 a.m. UTC | #4

> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Monday, July 02, 2018 11:18 PM
> To: Liu, Yong <yong.liu@intel.com>; Bie, Tiwei <tiwei.bie@intel.com>;
> Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: Wang, Zhihong <zhihong.wang@intel.com>; dev@dpdk.org
> Subject: Re: [PATCH v5 8/9] net/virtio: add in-order Rx/Tx into selection
> 
> 
> 
> On 07/02/2018 02:41 PM, Maxime Coquelin wrote:
> >
> >
> > On 07/02/2018 01:24 PM, Maxime Coquelin wrote:
> >>
> >>
> >> On 07/02/2018 03:56 PM, Marvin Liu wrote:
> >>> After IN_ORDER Rx/Tx paths added, need to update Rx/Tx path selection
> >>> logic.
> >>>
> >>> Rx path select logic: If IN_ORDER and merge-able are enabled will
> select
> >>> IN_ORDER Rx path. If IN_ORDER is enabled, Rx offload and merge-able
> are
> >>> disabled will select simple Rx path. Otherwise will select normal Rx
> >>> path.
> >>>
> >>> Tx path select logic: If IN_ORDER is enabled will select IN_ORDER Tx
> >>> path. Otherwise will select default Tx path.
> >>>
> >>> Signed-off-by: Marvin Liu <yong.liu@intel.com>
> >>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>
> >>> diff --git a/doc/guides/nics/virtio.rst b/doc/guides/nics/virtio.rst
> >>> index 46e292c4d..7c099fb7c 100644
> >>> --- a/doc/guides/nics/virtio.rst
> >>> +++ b/doc/guides/nics/virtio.rst
> >>> @@ -201,7 +201,7 @@ The packet transmission flow is:
> >>>   Virtio PMD Rx/Tx Callbacks
> >>>   --------------------------
> >>> -Virtio driver has 3 Rx callbacks and 2 Tx callbacks.
> >>> +Virtio driver has 4 Rx callbacks and 3 Tx callbacks.
> >>>   Rx callbacks:
> >>> @@ -215,6 +215,9 @@ Rx callbacks:
> >>>      Vector version without mergeable Rx buffer support, also fixes
> >>> the available
> >>>      ring indexes and uses vector instructions to optimize performance.
> >>> +#. ``virtio_recv_mergeable_pkts_inorder``:
> >>> +   In-order version with mergeable Rx buffer support.
> >>> +
> >>>   Tx callbacks:
> >>>   #. ``virtio_xmit_pkts``:
> >>> @@ -223,6 +226,8 @@ Tx callbacks:
> >>>   #. ``virtio_xmit_pkts_simple``:
> >>>      Vector version fixes the available ring indexes to optimize
> >>> performance.
> >>> +#. ``virtio_xmit_pkts_inorder``:
> >>> +   In-order version.
> >>>   By default, the non-vector callbacks are used:
> >>> @@ -254,6 +259,12 @@ Example of using the vector version of the
> >>> virtio poll mode driver in
> >>>      testpmd -l 0-2 -n 4 -- -i --tx-offloads=0x0 --rxq=1 --txq=1
> >>> --nb-cores=1
> >>> +In-order callbacks only work on simulated virtio user vdev.
> >>> +
> >>> +*   For Rx: If mergeable Rx buffers is enabled and in-order is
> >>> enabled then
> >>> +    ``virtio_xmit_pkts_inorder`` is used.
> >>> +
> >>> +*   For Tx: If in-order is enabled then ``virtio_xmit_pkts_inorder``
> >>> is used.
> >>>   Interrupt mode
> >>>   --------------
> >>> diff --git a/drivers/net/virtio/virtio_ethdev.c
> >>> b/drivers/net/virtio/virtio_ethdev.c
> >>> index df50a571a..df7981ddb 100644
> >>> --- a/drivers/net/virtio/virtio_ethdev.c
> >>> +++ b/drivers/net/virtio/virtio_ethdev.c
> >>> @@ -1320,6 +1320,11 @@ set_rxtx_funcs(struct rte_eth_dev *eth_dev)
> >>>           PMD_INIT_LOG(INFO, "virtio: using simple Rx path on port %u",
> >>>               eth_dev->data->port_id);
> >>>           eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
> >>> +    } else if (hw->use_inorder_rx) {
> >>> +        PMD_INIT_LOG(INFO,
> >>> +            "virtio: using inorder mergeable buffer Rx path on port
> >>> %u",
> >>> +            eth_dev->data->port_id);
> >>> +        eth_dev->rx_pkt_burst = &virtio_recv_mergeable_pkts_inorder;
> >>>       } else if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
> >>>           PMD_INIT_LOG(INFO,
> >>>               "virtio: using mergeable buffer Rx path on port %u",
> >>> @@ -1335,6 +1340,10 @@ set_rxtx_funcs(struct rte_eth_dev *eth_dev)
> >>>           PMD_INIT_LOG(INFO, "virtio: using simple Tx path on port %u",
> >>>               eth_dev->data->port_id);
> >>>           eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
> >>> +    } else if (hw->use_inorder_tx) {
> >>> +        PMD_INIT_LOG(INFO, "virtio: using inorder Tx path on port %u",
> >>> +            eth_dev->data->port_id);
> >>> +        eth_dev->tx_pkt_burst = virtio_xmit_pkts_inorder;
> >>>       } else {
> >>>           PMD_INIT_LOG(INFO, "virtio: using standard Tx path on port
> >>> %u",
> >>>               eth_dev->data->port_id);
> >>> @@ -1874,20 +1883,27 @@ virtio_dev_configure(struct rte_eth_dev *dev)
> >>>       hw->use_simple_rx = 1;
> >>>       hw->use_simple_tx = 1;
> >>> +    if (vtpci_with_feature(hw, VIRTIO_F_IN_ORDER)) {
> >>> +        /* Simple Tx not compatible with in-order ring */
> >>> +        hw->use_inorder_tx = 1;
> >>> +        hw->use_simple_tx = 0;
> >>> +        if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
> >>> +            hw->use_inorder_rx = 1;
> >>> +            hw->use_simple_rx = 0;
> >>> +        } else {
> >>> +            hw->use_inorder_rx = 0;
> >>> +            if (rx_offloads & (DEV_RX_OFFLOAD_UDP_CKSUM |
> >>> +                       DEV_RX_OFFLOAD_TCP_CKSUM))
> >>> +                hw->use_simple_rx = 0;
> >> It is applied, but I think this is still not good.
> >>
> >> Simple Rx is set to 1 by default, so if IN_ORDER isn't negotiated,
> >> and UDP/TCP_CSUM is enabled, simple Rx keeps being selected.
> >>
> >> I'll fix it in my series that I'm doing on top.
> >
> > Actually, after discussion with Ferruh, I fixed it directly in the patch.
> 
> Running some more tests, I noticed that it is still broken.
> For example the case where host does not support IN_ORDER.
> If mergeable buffer is negotiated, the simple path gets selected, which
> is wrong.
> 
> I fixed that directly in the patch.

Thanks Maxime. Sorry, I focused on in-order part and ignored original path check.

> 
> Regards,
> Maxime
> > Thanks,
> > Maxime
> >
> >> Regards,
> >> Maxime
> >>
> >>> +        }
> >>> +    }
> >>> +
> >>>   #if defined RTE_ARCH_ARM64 || defined RTE_ARCH_ARM
> >>>       if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON)) {
> >>>           hw->use_simple_rx = 0;
> >>>           hw->use_simple_tx = 0;
> >>>       }
> >>>   #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 |
> >>> -               DEV_RX_OFFLOAD_TCP_CKSUM))
> >>> -        hw->use_simple_rx = 0;
> >>>       return 0;
> >>>   }
> >>>

diff --git a/doc/guides/nics/virtio.rst b/doc/guides/nics/virtio.rst
index 46e292c4d..7c099fb7c 100644
--- a/doc/guides/nics/virtio.rst
+++ b/doc/guides/nics/virtio.rst
@@ -201,7 +201,7 @@  The packet transmission flow is:
 Virtio PMD Rx/Tx Callbacks
 --------------------------
 
-Virtio driver has 3 Rx callbacks and 2 Tx callbacks.
+Virtio driver has 4 Rx callbacks and 3 Tx callbacks.
 
 Rx callbacks:
 
@@ -215,6 +215,9 @@  Rx callbacks:
    Vector version without mergeable Rx buffer support, also fixes the available
    ring indexes and uses vector instructions to optimize performance.
 
+#. ``virtio_recv_mergeable_pkts_inorder``:
+   In-order version with mergeable Rx buffer support.
+
 Tx callbacks:
 
 #. ``virtio_xmit_pkts``:
@@ -223,6 +226,8 @@  Tx callbacks:
 #. ``virtio_xmit_pkts_simple``:
    Vector version fixes the available ring indexes to optimize performance.
 
+#. ``virtio_xmit_pkts_inorder``:
+   In-order version.
 
 By default, the non-vector callbacks are used:
 
@@ -254,6 +259,12 @@  Example of using the vector version of the virtio poll mode driver in
 
    testpmd -l 0-2 -n 4 -- -i --tx-offloads=0x0 --rxq=1 --txq=1 --nb-cores=1
 
+In-order callbacks only work on simulated virtio user vdev.
+
+*   For Rx: If mergeable Rx buffers is enabled and in-order is enabled then
+    ``virtio_xmit_pkts_inorder`` is used.
+
+*   For Tx: If in-order is enabled then ``virtio_xmit_pkts_inorder`` is used.
 
 Interrupt mode
 --------------
diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index df50a571a..df7981ddb 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1320,6 +1320,11 @@  set_rxtx_funcs(struct rte_eth_dev *eth_dev)
 		PMD_INIT_LOG(INFO, "virtio: using simple Rx path on port %u",
 			eth_dev->data->port_id);
 		eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
+	} else if (hw->use_inorder_rx) {
+		PMD_INIT_LOG(INFO,
+			"virtio: using inorder mergeable buffer Rx path on port %u",
+			eth_dev->data->port_id);
+		eth_dev->rx_pkt_burst = &virtio_recv_mergeable_pkts_inorder;
 	} else if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
 		PMD_INIT_LOG(INFO,
 			"virtio: using mergeable buffer Rx path on port %u",
@@ -1335,6 +1340,10 @@  set_rxtx_funcs(struct rte_eth_dev *eth_dev)
 		PMD_INIT_LOG(INFO, "virtio: using simple Tx path on port %u",
 			eth_dev->data->port_id);
 		eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
+	} else if (hw->use_inorder_tx) {
+		PMD_INIT_LOG(INFO, "virtio: using inorder Tx path on port %u",
+			eth_dev->data->port_id);
+		eth_dev->tx_pkt_burst = virtio_xmit_pkts_inorder;
 	} else {
 		PMD_INIT_LOG(INFO, "virtio: using standard Tx path on port %u",
 			eth_dev->data->port_id);
@@ -1874,20 +1883,27 @@  virtio_dev_configure(struct rte_eth_dev *dev)
 	hw->use_simple_rx = 1;
 	hw->use_simple_tx = 1;
 
+	if (vtpci_with_feature(hw, VIRTIO_F_IN_ORDER)) {
+		/* Simple Tx not compatible with in-order ring */
+		hw->use_inorder_tx = 1;
+		hw->use_simple_tx = 0;
+		if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
+			hw->use_inorder_rx = 1;
+			hw->use_simple_rx = 0;
+		} else {
+			hw->use_inorder_rx = 0;
+			if (rx_offloads & (DEV_RX_OFFLOAD_UDP_CKSUM |
+					   DEV_RX_OFFLOAD_TCP_CKSUM))
+				hw->use_simple_rx = 0;
+		}
+	}
+
 #if defined RTE_ARCH_ARM64 || defined RTE_ARCH_ARM
 	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON)) {
 		hw->use_simple_rx = 0;
 		hw->use_simple_tx = 0;
 	}
 #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 |
-			   DEV_RX_OFFLOAD_TCP_CKSUM))
-		hw->use_simple_rx = 0;
 
 	return 0;
 }

[v5,8/9] net/virtio: add in-order Rx/Tx into selection

Checks

Commit Message

Comments

Patch