diff mbox series

vhost: make iotlb cache name unique among multi processes

Message ID 20200310050003.16728-1-oda@valinux.co.jp (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series vhost: make iotlb cache name unique among multi processes |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/travis-robot success Travis build: passed
ci/Intel-compilation success Compilation OK

Commit Message

Itsuro Oda March 10, 2020, 5 a.m. UTC 
  Currently, iotlb cache name is comprised of vid and virtqueue
index. For example, "iotlb_cache_0_0". Because vid is assigned
per process, iotlb cache name is not unique among multi processes.
For example a secondary process uses a vhost
(ex. eth_vhost0,iface=/tmp/sock0) and another secondary process
uses a vhost (ex. eth_vhost1,iface=/tmp/sock1), iotlb cache
name of both vhost ("iotlb_cache_0_0") are same and as a result
iotlb cache is broken.

This patch makes iotlb cache name unique among milti processes
by using the interface name not vid to comprise iotlb cache name.
Since the length of interface name is variable, this patch uses
hash value calculated by the interface name.

Fixes: d012d1f293f4 (vhost: add IOTLB helper functions)
Cc: stable@dpdk.org

Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
---
 lib/librte_vhost/iotlb.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Xiaolong Ye March 10, 2020, 11:32 a.m. UTC | #1 
On 03/10, Itsuro Oda wrote:
>Currently, iotlb cache name is comprised of vid and virtqueue
>index. For example, "iotlb_cache_0_0". Because vid is assigned
>per process, iotlb cache name is not unique among multi processes.
>For example a secondary process uses a vhost
>(ex. eth_vhost0,iface=/tmp/sock0) and another secondary process
>uses a vhost (ex. eth_vhost1,iface=/tmp/sock1), iotlb cache
>name of both vhost ("iotlb_cache_0_0") are same and as a result
>iotlb cache is broken.
>
>This patch makes iotlb cache name unique among milti processes
>by using the interface name not vid to comprise iotlb cache name.
>Since the length of interface name is variable, this patch uses
>hash value calculated by the interface name.
>
>Fixes: d012d1f293f4 (vhost: add IOTLB helper functions)
>Cc: stable@dpdk.org
>
>Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
>---
> lib/librte_vhost/iotlb.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
>diff --git a/lib/librte_vhost/iotlb.c b/lib/librte_vhost/iotlb.c
>index bc1758528..0992c145b 100644
>--- a/lib/librte_vhost/iotlb.c
>+++ b/lib/librte_vhost/iotlb.c
>@@ -6,6 +6,7 @@
> #include <numaif.h>
> #endif
> 
>+#include <rte_jhash.h>
> #include <rte_tailq.h>
> 
> #include "iotlb.h"
>@@ -288,6 +289,7 @@ vhost_user_iotlb_init(struct virtio_net *dev, int vq_index)
> 	char pool_name[RTE_MEMPOOL_NAMESIZE];
> 	struct vhost_virtqueue *vq = dev->virtqueue[vq_index];
> 	int socket = 0;
>+	uint32_t val;
> 
> 	if (vq->iotlb_pool) {
> 		/*
>@@ -308,8 +310,10 @@ vhost_user_iotlb_init(struct virtio_net *dev, int vq_index)
> 	TAILQ_INIT(&vq->iotlb_list);
> 	TAILQ_INIT(&vq->iotlb_pending_list);
> 
>-	snprintf(pool_name, sizeof(pool_name), "iotlb_cache_%d_%d",
>-			dev->vid, vq_index);
>+	val = rte_jhash(dev->ifname, strlen(dev->ifname), 0);
>+	snprintf(pool_name, sizeof(pool_name), "iotlb_cache_%08x_%d",
>+			val, vq_index);
>+	VHOST_LOG_CONFIG(DEBUG, "IOTLB cache name: %s\n", pool_name);
> 
> 	/* If already created, free it and recreate */
> 	vq->iotlb_pool = rte_mempool_lookup(pool_name);
>-- 
>2.17.0
>

Acked-by: Xiaolong Ye <xiaolong.ye@intel.com>
Van Haaren, Harry March 10, 2020, 12:44 p.m. UTC | #2 
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Ye Xiaolong
> Sent: Tuesday, March 10, 2020 11:32 AM
> To: Itsuro Oda <oda@valinux.co.jp>
> Cc: dev@dpdk.org; maxime.coquelin@redhat.com; Wang, Zhihong
> <zhihong.wang@intel.com>; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] vhost: make iotlb cache name unique among
> multi processes
> 
> On 03/10, Itsuro Oda wrote:
> >Currently, iotlb cache name is comprised of vid and virtqueue
> >index. For example, "iotlb_cache_0_0". Because vid is assigned
> >per process, iotlb cache name is not unique among multi processes.
> >For example a secondary process uses a vhost
> >(ex. eth_vhost0,iface=/tmp/sock0) and another secondary process
> >uses a vhost (ex. eth_vhost1,iface=/tmp/sock1), iotlb cache
> >name of both vhost ("iotlb_cache_0_0") are same and as a result
> >iotlb cache is broken.
> >
> >This patch makes iotlb cache name unique among milti processes
> >by using the interface name not vid to comprise iotlb cache name.
> >Since the length of interface name is variable, this patch uses
> >hash value calculated by the interface name.
> >
> >Fixes: d012d1f293f4 (vhost: add IOTLB helper functions)
> >Cc: stable@dpdk.org
> >
> >Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
> >---
> > lib/librte_vhost/iotlb.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> >diff --git a/lib/librte_vhost/iotlb.c b/lib/librte_vhost/iotlb.c
> >index bc1758528..0992c145b 100644
> >--- a/lib/librte_vhost/iotlb.c
> >+++ b/lib/librte_vhost/iotlb.c
> >@@ -6,6 +6,7 @@
> > #include <numaif.h>
> > #endif
> >
> >+#include <rte_jhash.h>
> > #include <rte_tailq.h>
> >
> > #include "iotlb.h"
> >@@ -288,6 +289,7 @@ vhost_user_iotlb_init(struct virtio_net *dev, int
> vq_index)
> > 	char pool_name[RTE_MEMPOOL_NAMESIZE];
> > 	struct vhost_virtqueue *vq = dev->virtqueue[vq_index];
> > 	int socket = 0;
> >+	uint32_t val;
> >
> > 	if (vq->iotlb_pool) {
> > 		/*
> >@@ -308,8 +310,10 @@ vhost_user_iotlb_init(struct virtio_net *dev, int
> vq_index)
> > 	TAILQ_INIT(&vq->iotlb_list);
> > 	TAILQ_INIT(&vq->iotlb_pending_list);
> >
> >-	snprintf(pool_name, sizeof(pool_name), "iotlb_cache_%d_%d",
> >-			dev->vid, vq_index);
> >+	val = rte_jhash(dev->ifname, strlen(dev->ifname), 0);
> >+	snprintf(pool_name, sizeof(pool_name), "iotlb_cache_%08x_%d",
> >+			val, vq_index);
> >+	VHOST_LOG_CONFIG(DEBUG, "IOTLB cache name: %s\n", pool_name);

Although very unlikely, what would happen if there is a hash-collision?

For example imagine two different names hash to the same "val", from
my understanding they will now use the same IOTLB but should not share one.

<snip>
Maxime Coquelin March 10, 2020, 12:58 p.m. UTC | #3 
On 3/10/20 1:44 PM, Van Haaren, Harry wrote:
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Ye Xiaolong
>> Sent: Tuesday, March 10, 2020 11:32 AM
>> To: Itsuro Oda <oda@valinux.co.jp>
>> Cc: dev@dpdk.org; maxime.coquelin@redhat.com; Wang, Zhihong
>> <zhihong.wang@intel.com>; stable@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] vhost: make iotlb cache name unique among
>> multi processes
>>
>> On 03/10, Itsuro Oda wrote:
>>> Currently, iotlb cache name is comprised of vid and virtqueue
>>> index. For example, "iotlb_cache_0_0". Because vid is assigned
>>> per process, iotlb cache name is not unique among multi processes.
>>> For example a secondary process uses a vhost
>>> (ex. eth_vhost0,iface=/tmp/sock0) and another secondary process
>>> uses a vhost (ex. eth_vhost1,iface=/tmp/sock1), iotlb cache
>>> name of both vhost ("iotlb_cache_0_0") are same and as a result
>>> iotlb cache is broken.
>>>
>>> This patch makes iotlb cache name unique among milti processes
>>> by using the interface name not vid to comprise iotlb cache name.
>>> Since the length of interface name is variable, this patch uses
>>> hash value calculated by the interface name.
>>>
>>> Fixes: d012d1f293f4 (vhost: add IOTLB helper functions)
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
>>> ---
>>> lib/librte_vhost/iotlb.c | 8 ++++++--
>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/librte_vhost/iotlb.c b/lib/librte_vhost/iotlb.c
>>> index bc1758528..0992c145b 100644
>>> --- a/lib/librte_vhost/iotlb.c
>>> +++ b/lib/librte_vhost/iotlb.c
>>> @@ -6,6 +6,7 @@
>>> #include <numaif.h>
>>> #endif
>>>
>>> +#include <rte_jhash.h>
>>> #include <rte_tailq.h>
>>>
>>> #include "iotlb.h"
>>> @@ -288,6 +289,7 @@ vhost_user_iotlb_init(struct virtio_net *dev, int
>> vq_index)
>>> 	char pool_name[RTE_MEMPOOL_NAMESIZE];
>>> 	struct vhost_virtqueue *vq = dev->virtqueue[vq_index];
>>> 	int socket = 0;
>>> +	uint32_t val;
>>>
>>> 	if (vq->iotlb_pool) {
>>> 		/*
>>> @@ -308,8 +310,10 @@ vhost_user_iotlb_init(struct virtio_net *dev, int
>> vq_index)
>>> 	TAILQ_INIT(&vq->iotlb_list);
>>> 	TAILQ_INIT(&vq->iotlb_pending_list);
>>>
>>> -	snprintf(pool_name, sizeof(pool_name), "iotlb_cache_%d_%d",
>>> -			dev->vid, vq_index);
>>> +	val = rte_jhash(dev->ifname, strlen(dev->ifname), 0);
>>> +	snprintf(pool_name, sizeof(pool_name), "iotlb_cache_%08x_%d",
>>> +			val, vq_index);
>>> +	VHOST_LOG_CONFIG(DEBUG, "IOTLB cache name: %s\n", pool_name);
> 
> Although very unlikely, what would happen if there is a hash-collision?
> 
> For example imagine two different names hash to the same "val", from
> my understanding they will now use the same IOTLB but should not share one.
> 
> <snip>
> 

+1.

Instead of a hash, maybe use the process ID:
snprintf(pool_name, sizeof(pool_name), "iotlb_cache_%u_%d_%d",
	 pid, dev->vid, vq_index);
diff mbox series

Patch

diff --git a/lib/librte_vhost/iotlb.c b/lib/librte_vhost/iotlb.c
index bc1758528..0992c145b 100644
--- a/lib/librte_vhost/iotlb.c
+++ b/lib/librte_vhost/iotlb.c
@@ -6,6 +6,7 @@ 
 #include <numaif.h>
 #endif
 
+#include <rte_jhash.h>
 #include <rte_tailq.h>
 
 #include "iotlb.h"
@@ -288,6 +289,7 @@  vhost_user_iotlb_init(struct virtio_net *dev, int vq_index)
 	char pool_name[RTE_MEMPOOL_NAMESIZE];
 	struct vhost_virtqueue *vq = dev->virtqueue[vq_index];
 	int socket = 0;
+	uint32_t val;
 
 	if (vq->iotlb_pool) {
 		/*
@@ -308,8 +310,10 @@  vhost_user_iotlb_init(struct virtio_net *dev, int vq_index)
 	TAILQ_INIT(&vq->iotlb_list);
 	TAILQ_INIT(&vq->iotlb_pending_list);
 
-	snprintf(pool_name, sizeof(pool_name), "iotlb_cache_%d_%d",
-			dev->vid, vq_index);
+	val = rte_jhash(dev->ifname, strlen(dev->ifname), 0);
+	snprintf(pool_name, sizeof(pool_name), "iotlb_cache_%08x_%d",
+			val, vq_index);
+	VHOST_LOG_CONFIG(DEBUG, "IOTLB cache name: %s\n", pool_name);
 
 	/* If already created, free it and recreate */
 	vq->iotlb_pool = rte_mempool_lookup(pool_name);