diff mbox

[dpdk-dev] vhost: make vhost lockless enqueue configurable

Message ID 1430306974-9618-1-git-send-email-huawei.xie@intel.com (mailing list archive)
State Changes Requested, archived
Headers

Commit Message

Huawei Xie April 29, 2015, 11:29 a.m. UTC 
  vhost enabled vSwitch could have their own thread-safe vring enqueue policy.
Add the RTE_LIBRTE_VHOST_LOCKLESS_ENQ macro for vhost lockless enqueue.
Turn it off by default.

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
---
 config/common_linuxapp        |  1 +
 lib/librte_vhost/vhost_rxtx.c | 24 +++++++++++++++++++++++-
 2 files changed, 24 insertions(+), 1 deletion(-)

Comments

Panu Matilainen April 29, 2015, 11:38 a.m. UTC | #1 
On 04/29/2015 02:29 PM, Huawei Xie wrote:
> vhost enabled vSwitch could have their own thread-safe vring enqueue policy.
> Add the RTE_LIBRTE_VHOST_LOCKLESS_ENQ macro for vhost lockless enqueue.
> Turn it off by default.
>
> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> ---
>   config/common_linuxapp        |  1 +
>   lib/librte_vhost/vhost_rxtx.c | 24 +++++++++++++++++++++++-
>   2 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/config/common_linuxapp b/config/common_linuxapp
> index 0078dc9..7f59499 100644
> --- a/config/common_linuxapp
> +++ b/config/common_linuxapp
> @@ -421,6 +421,7 @@ CONFIG_RTE_KNI_VHOST_DEBUG_TX=n
>   #
>   CONFIG_RTE_LIBRTE_VHOST=n
>   CONFIG_RTE_LIBRTE_VHOST_USER=y
> +CONFIG_RTE_LIBRTE_VHOST_LOCKLESS_ENQ=n
>   CONFIG_RTE_LIBRTE_VHOST_DEBUG=n
>
>   #
> diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
> index 510ffe8..475be6e 100644
> --- a/lib/librte_vhost/vhost_rxtx.c
> +++ b/lib/librte_vhost/vhost_rxtx.c
> @@ -80,7 +80,11 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
>   	 * they need to be reserved.
>   	 */
>   	do {
> +#ifdef RTE_LIBRTE_VHOST_LOCKESS_ENQ
>   		res_base_idx = vq->last_used_idx_res;
> +#else
> +		res_base_idx = vq->last_used_idx;
> +#endif

These things should be runtime configurable, not build options. Please 
do not assume everybody builds DPDK separately for each and every 
application that might ever be.

	- Panu -
Thomas Monjalon April 29, 2015, 11:56 a.m. UTC | #2 
2015-04-29 13:38 GMT+02:00 Panu Matilainen <pmatilai@redhat.com>:
> On 04/29/2015 02:29 PM, Huawei Xie wrote:
>>
>> vhost enabled vSwitch could have their own thread-safe vring enqueue
>> policy.
>> Add the RTE_LIBRTE_VHOST_LOCKLESS_ENQ macro for vhost lockless enqueue.
>> Turn it off by default.
>>
>> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
>> ---
>>   config/common_linuxapp        |  1 +
>>   lib/librte_vhost/vhost_rxtx.c | 24 +++++++++++++++++++++++-
>>   2 files changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/config/common_linuxapp b/config/common_linuxapp
>> index 0078dc9..7f59499 100644
>> --- a/config/common_linuxapp
>> +++ b/config/common_linuxapp
>> @@ -421,6 +421,7 @@ CONFIG_RTE_KNI_VHOST_DEBUG_TX=n
>>   #
>>   CONFIG_RTE_LIBRTE_VHOST=n
>>   CONFIG_RTE_LIBRTE_VHOST_USER=y
>> +CONFIG_RTE_LIBRTE_VHOST_LOCKLESS_ENQ=n
>>   CONFIG_RTE_LIBRTE_VHOST_DEBUG=n
[...]
>
> These things should be runtime configurable, not build options. Please do
> not assume everybody builds DPDK separately for each and every application
> that might ever be.

+1
Adding new build options must be exceptions and very well justified.
Flavio Leitner April 29, 2015, 6:07 p.m. UTC | #3 
On Wed, 29 Apr 2015 13:56:58 +0200
Thomas Monjalon <thomas.monjalon@6wind.com> wrote:

> 2015-04-29 13:38 GMT+02:00 Panu Matilainen <pmatilai@redhat.com>:
> > On 04/29/2015 02:29 PM, Huawei Xie wrote:
> >>
> >> vhost enabled vSwitch could have their own thread-safe vring
> >> enqueue policy.
> >> Add the RTE_LIBRTE_VHOST_LOCKLESS_ENQ macro for vhost lockless
> >> enqueue. Turn it off by default.
> >>
> >> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> >> ---
> >>   config/common_linuxapp        |  1 +
> >>   lib/librte_vhost/vhost_rxtx.c | 24 +++++++++++++++++++++++-
> >>   2 files changed, 24 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/config/common_linuxapp b/config/common_linuxapp
> >> index 0078dc9..7f59499 100644
> >> --- a/config/common_linuxapp
> >> +++ b/config/common_linuxapp
> >> @@ -421,6 +421,7 @@ CONFIG_RTE_KNI_VHOST_DEBUG_TX=n
> >>   #
> >>   CONFIG_RTE_LIBRTE_VHOST=n
> >>   CONFIG_RTE_LIBRTE_VHOST_USER=y
> >> +CONFIG_RTE_LIBRTE_VHOST_LOCKLESS_ENQ=n
> >>   CONFIG_RTE_LIBRTE_VHOST_DEBUG=n
> [...]
> >
> > These things should be runtime configurable, not build options.
> > Please do not assume everybody builds DPDK separately for each and
> > every application that might ever be.
> 
> +1
> Adding new build options must be exceptions and very well justified.

+1, that's specially hard when dpdk is packaged and distributed on
distros like Fedora/RHEL.

fbl
Huawei Xie May 21, 2015, 8:49 a.m. UTC | #4 
On 4/29/2015 7:57 PM, Thomas Monjalon wrote:
> 2015-04-29 13:38 GMT+02:00 Panu Matilainen <pmatilai@redhat.com>:
>> On 04/29/2015 02:29 PM, Huawei Xie wrote:
>>> vhost enabled vSwitch could have their own thread-safe vring enqueue
>>> policy.
>>> Add the RTE_LIBRTE_VHOST_LOCKLESS_ENQ macro for vhost lockless enqueue.
>>> Turn it off by default.
>>>
>>> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
>>> ---
>>>   config/common_linuxapp        |  1 +
>>>   lib/librte_vhost/vhost_rxtx.c | 24 +++++++++++++++++++++++-
>>>   2 files changed, 24 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/config/common_linuxapp b/config/common_linuxapp
>>> index 0078dc9..7f59499 100644
>>> --- a/config/common_linuxapp
>>> +++ b/config/common_linuxapp
>>> @@ -421,6 +421,7 @@ CONFIG_RTE_KNI_VHOST_DEBUG_TX=n
>>>   #
>>>   CONFIG_RTE_LIBRTE_VHOST=n
>>>   CONFIG_RTE_LIBRTE_VHOST_USER=y
>>> +CONFIG_RTE_LIBRTE_VHOST_LOCKLESS_ENQ=n
>>>   CONFIG_RTE_LIBRTE_VHOST_DEBUG=n
> [...]
>> These things should be runtime configurable, not build options. Please do
>> not assume everybody builds DPDK separately for each and every application
>> that might ever be.
> +1
> Adding new build options must be exceptions and very well justified.

Thomas:
Though we could make per queue/device run time configurable, i feel it
might not make sense that per queue/device has different behavior.
 To ease the implementation, i would provide an API to configure the
global lock less behavior, and by default, lockess enqueue will be disabled.
Thoughts?

>
>
diff mbox

Patch

diff --git a/config/common_linuxapp b/config/common_linuxapp
index 0078dc9..7f59499 100644
--- a/config/common_linuxapp
+++ b/config/common_linuxapp
@@ -421,6 +421,7 @@  CONFIG_RTE_KNI_VHOST_DEBUG_TX=n
 #
 CONFIG_RTE_LIBRTE_VHOST=n
 CONFIG_RTE_LIBRTE_VHOST_USER=y
+CONFIG_RTE_LIBRTE_VHOST_LOCKLESS_ENQ=n
 CONFIG_RTE_LIBRTE_VHOST_DEBUG=n
 
 #
diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
index 510ffe8..475be6e 100644
--- a/lib/librte_vhost/vhost_rxtx.c
+++ b/lib/librte_vhost/vhost_rxtx.c
@@ -80,7 +80,11 @@  virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
 	 * they need to be reserved.
 	 */
 	do {
+#ifdef RTE_LIBRTE_VHOST_LOCKESS_ENQ
 		res_base_idx = vq->last_used_idx_res;
+#else
+		res_base_idx = vq->last_used_idx;
+#endif
 		avail_idx = *((volatile uint16_t *)&vq->avail->idx);
 
 		free_entries = (avail_idx - res_base_idx);
@@ -92,10 +96,15 @@  virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
 			return 0;
 
 		res_end_idx = res_base_idx + count;
+
+#ifdef RTE_LIBRTE_VHOST_LOCKLESS_ENQ
 		/* vq->last_used_idx_res is atomically updated. */
-		/* TODO: Allow to disable cmpset if no concurrency in application. */
 		success = rte_atomic16_cmpset(&vq->last_used_idx_res,
 				res_base_idx, res_end_idx);
+#else
+		/* last_used_idx_res isn't used. */
+		success = 1;
+#endif
 	} while (unlikely(success == 0));
 	res_cur_idx = res_base_idx;
 	LOG_DEBUG(VHOST_DATA, "(%"PRIu64") Current Index %d| End Index %d\n",
@@ -171,9 +180,11 @@  virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
 
 	rte_compiler_barrier();
 
+#ifdef RTE_LIBRTE_VHOST_LOCKLESS_ENQ
 	/* Wait until it's our turn to add our buffer to the used ring. */
 	while (unlikely(vq->last_used_idx != res_base_idx))
 		rte_pause();
+#endif
 
 	*(volatile uint16_t *)&vq->used->idx += count;
 	vq->last_used_idx = res_end_idx;
@@ -422,11 +433,15 @@  virtio_dev_merge_rx(struct virtio_net *dev, uint16_t queue_id,
 		uint16_t i, id;
 
 		do {
+#ifdef RTE_LIBRTE_VHOST_LOCKLESS_ENQ
 			/*
 			 * As many data cores may want access to available
 			 * buffers, they need to be reserved.
 			 */
 			res_base_idx = vq->last_used_idx_res;
+#else
+			res_base_idx = vq->last_used_idx;
+#endif
 			res_cur_idx = res_base_idx;
 
 			do {
@@ -459,10 +474,15 @@  virtio_dev_merge_rx(struct virtio_net *dev, uint16_t queue_id,
 				}
 			} while (pkt_len > secure_len);
 
+#ifdef RTE_LIBRTE_VHOST_LOCKLESS_ENQ
 			/* vq->last_used_idx_res is atomically updated. */
 			success = rte_atomic16_cmpset(&vq->last_used_idx_res,
 							res_base_idx,
 							res_cur_idx);
+#else
+			/* last_used_idx_res isn't used. */
+			success = 1;
+#endif
 		} while (success == 0);
 
 		id = res_base_idx;
@@ -495,12 +515,14 @@  virtio_dev_merge_rx(struct virtio_net *dev, uint16_t queue_id,
 
 		rte_compiler_barrier();
 
+#ifdef RTE_LIBRTE_VHOST_LOCKLESS_ENQ
 		/*
 		 * Wait until it's our turn to add our buffer
 		 * to the used ring.
 		 */
 		while (unlikely(vq->last_used_idx != res_base_idx))
 			rte_pause();
+#endif
 
 		*(volatile uint16_t *)&vq->used->idx += entry_success;
 		vq->last_used_idx = res_end_idx;