[dpdk-dev] [PATCH] vhost: initializing iotlb memory only when IOMMU feature is enabled.

[dpdk-dev] [PATCH] vhost: initializing iotlb memory only when IOMMU feature is enabled.

[Nitin Katiyar]

DPDK 17.11 introduced the IOMMU feature which caused additional
DPDK memory requirement per vhostuser device as part of
iotlb_init(). Today this is done unconditionally (from DPDK
17.11 onwards) i.e. irrespective of IOMMU feature being
enabled on the vhostuser device, iotlb is initialized. This
breaks the backward compatibility for applications like OVS
due to increase in the DPDK memory footprint and causes upgrade
failures.

This patch is to do iotlb_init only if IOMMU feature is
enabled on device.

Signed-off-by: Nitin Katiyar <nitin.katiyar@ericsson.com>
---
 lib/librte_vhost/iotlb.c      | 7 +++++++
 lib/librte_vhost/vhost_user.c | 6 ++++++
 2 files changed, 13 insertions(+)

diff --git a/lib/librte_vhost/iotlb.c b/lib/librte_vhost/iotlb.c
index c6354fe..befa2d3 100644
--- a/lib/librte_vhost/iotlb.c
+++ b/lib/librte_vhost/iotlb.c
@@ -317,6 +317,13 @@ struct vhost_iotlb_entry {
 	struct vhost_virtqueue *vq = dev->virtqueue[vq_index];
 	int socket = 0;
 
+	if (!(dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM))) {
+		RTE_LOG(INFO, VHOST_CONFIG,
+			"IOMMU feature is not enabled for this dev(%s)\n",
+			dev->ifname);
+		return 0;
+	}
+
 	if (vq->iotlb_pool) {
 		/*
 		 * The cache has already been initialized,
diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index a2d4c9f..7553a03 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -1366,6 +1366,12 @@
 	uint16_t i;
 	uint64_t vva, len;
 
+	if (!(dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM))) {
+		RTE_LOG(ERR, VHOST_CONFIG,
+			"IOMMU feature is not enabled for this dev(%s)\n",
+			dev->ifname);
+		return -1;
+	}
 	switch (imsg->type) {
 	case VHOST_IOTLB_UPDATE:
 		len = imsg->size;
-- 
1.9.1

Re: [dpdk-dev] vhost: initializing iotlb memory only when IOMMU feature is enabled.

[Ilya Maximets]

On 17.08.2018 23:19, Nitin Katiyar wrote:
> DPDK 17.11 introduced the IOMMU feature which caused additional
> DPDK memory requirement per vhostuser device as part of
> iotlb_init(). Today this is done unconditionally (from DPDK
> 17.11 onwards) i.e. irrespective of IOMMU feature being
> enabled on the vhostuser device, iotlb is initialized. This
> breaks the backward compatibility for applications like OVS
> due to increase in the DPDK memory footprint and causes upgrade
> failures.
>> This patch is to do iotlb_init only if IOMMU feature is
> enabled on device.

I guess, "Fixes" line should be here?

> 
> Signed-off-by: Nitin Katiyar <nitin.katiyar@ericsson.com>
> ---
>  lib/librte_vhost/iotlb.c      | 7 +++++++
>  lib/librte_vhost/vhost_user.c | 6 ++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/lib/librte_vhost/iotlb.c b/lib/librte_vhost/iotlb.c
> index c6354fe..befa2d3 100644
> --- a/lib/librte_vhost/iotlb.c
> +++ b/lib/librte_vhost/iotlb.c
> @@ -317,6 +317,13 @@ struct vhost_iotlb_entry {
>  	struct vhost_virtqueue *vq = dev->virtqueue[vq_index];
>  	int socket = 0;
>  
> +	if (!(dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM))) {

vhost_user_iotlb_init() is called on the virtqueue allocation
stage, most likely while processing VHOST_USER_SET_VRING_CALL.
QEMU usually sends VHOST_USER_SET_FEATURES after the
VHOST_USER_SET_VRING_CALL. This means that 'dev->features' are
not yet initialized here.

Have you tested that IOMMU feature works with this patch applied?

> +		RTE_LOG(INFO, VHOST_CONFIG,
> +			"IOMMU feature is not enabled for this dev(%s)\n",
> +			dev->ifname);
> +		return 0;
> +	}
> +
>  	if (vq->iotlb_pool) {
>  		/*
>  		 * The cache has already been initialized,
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index a2d4c9f..7553a03 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -1366,6 +1366,12 @@
>  	uint16_t i;
>  	uint64_t vva, len;
>  
> +	if (!(dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM))) {
> +		RTE_LOG(ERR, VHOST_CONFIG,
> +			"IOMMU feature is not enabled for this dev(%s)\n",
> +			dev->ifname);
> +		return -1;
> +	}
>  	switch (imsg->type) {
>  	case VHOST_IOTLB_UPDATE:
>  		len = imsg->size;
> 

Re: [dpdk-dev] vhost: initializing iotlb memory only when IOMMU feature is enabled.

[Nitin Katiyar]

Hi Ilya,
Thanks for your comments. I haven't tested with IOMMU enabled. I covered more of memory utilization by testing OVS with DPDK. I will test out by enabling IOMMU and send the updated patch.

Regards,
Nitin

-----Original Message-----
From: Ilya Maximets [mailto:i.maximets@samsung.com] 
Sent: Friday, August 17, 2018 7:36 PM
To: Nitin Katiyar <nitin.katiyar@ericsson.com>; dev@dpdk.org
Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; Bie, Tiwei <tiwei.bie@intel.com>; Zhihong Wang <zhihong.wang@intel.com>
Subject: Re: vhost: initializing iotlb memory only when IOMMU feature is enabled.

On 17.08.2018 23:19, Nitin Katiyar wrote:
> DPDK 17.11 introduced the IOMMU feature which caused additional DPDK 
> memory requirement per vhostuser device as part of iotlb_init(). Today 
> this is done unconditionally (from DPDK
> 17.11 onwards) i.e. irrespective of IOMMU feature being enabled on the 
> vhostuser device, iotlb is initialized. This breaks the backward 
> compatibility for applications like OVS due to increase in the DPDK 
> memory footprint and causes upgrade failures.
>> This patch is to do iotlb_init only if IOMMU feature is
> enabled on device.

I guess, "Fixes" line should be here?

> 
> Signed-off-by: Nitin Katiyar <nitin.katiyar@ericsson.com>
> ---
>  lib/librte_vhost/iotlb.c      | 7 +++++++
>  lib/librte_vhost/vhost_user.c | 6 ++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/lib/librte_vhost/iotlb.c b/lib/librte_vhost/iotlb.c index 
> c6354fe..befa2d3 100644
> --- a/lib/librte_vhost/iotlb.c
> +++ b/lib/librte_vhost/iotlb.c
> @@ -317,6 +317,13 @@ struct vhost_iotlb_entry {
>  	struct vhost_virtqueue *vq = dev->virtqueue[vq_index];
>  	int socket = 0;
>  
> +	if (!(dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM))) {

vhost_user_iotlb_init() is called on the virtqueue allocation stage, most likely while processing VHOST_USER_SET_VRING_CALL.
QEMU usually sends VHOST_USER_SET_FEATURES after the VHOST_USER_SET_VRING_CALL. This means that 'dev->features' are not yet initialized here.

Have you tested that IOMMU feature works with this patch applied?

> +		RTE_LOG(INFO, VHOST_CONFIG,
> +			"IOMMU feature is not enabled for this dev(%s)\n",
> +			dev->ifname);
> +		return 0;
> +	}
> +
>  	if (vq->iotlb_pool) {
>  		/*
>  		 * The cache has already been initialized, diff --git 
> a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c index 
> a2d4c9f..7553a03 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -1366,6 +1366,12 @@
>  	uint16_t i;
>  	uint64_t vva, len;
>  
> +	if (!(dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM))) {
> +		RTE_LOG(ERR, VHOST_CONFIG,
> +			"IOMMU feature is not enabled for this dev(%s)\n",
> +			dev->ifname);
> +		return -1;
> +	}
>  	switch (imsg->type) {
>  	case VHOST_IOTLB_UPDATE:
>  		len = imsg->size;
> 

[dpdk-dev] [PATCH] vhost: initializing iotlb memory only when IOMMU feature is enabled

[Nitin Katiyar]

DPDK 17.11 introduced the IOMMU feature which caused additional DPDK
 memory requirement per vhostuser device as part of iotlb_init(). Today this
 is done unconditionally (from DPDK 17.11 onwards) i.e. irrespective of IOMMU
 feature being enabled on the vhostuser device, iotlb is initialized. This
 breaks the backward compatibility for applications like OVS due to increase
 in the DPDK memory footprint and causes upgrade failures.

This patch is to do iotlb_init only if IOMMU feature is enabled on device.

Signed-off-by: Nitin Katiyar <nitin.katiyar@ericsson.com>
---
 lib/librte_vhost/iotlb.c      | 7 +++++++
 lib/librte_vhost/vhost_user.c | 7 +++++++
 2 files changed, 14 insertions(+)

diff --git a/lib/librte_vhost/iotlb.c b/lib/librte_vhost/iotlb.c
index c11ebca..0d7c820 100644
--- a/lib/librte_vhost/iotlb.c
+++ b/lib/librte_vhost/iotlb.c
@@ -310,6 +310,13 @@ struct vhost_iotlb_entry {
 	struct vhost_virtqueue *vq = dev->virtqueue[vq_index];
 	int socket = 0;
 
+	if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM)) {
+		RTE_LOG(ERR, VHOST_CONFIG,
+			"IOMMU feature is not enabled for this dev(%s)\n",
+			dev->ifname);
+		return -1;
+	}
+
 	if (vq->iotlb_pool) {
 		/*
 		 * The cache has already been initialized,
diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 26cfebe..ca787f3 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -1274,6 +1274,13 @@
 	uint16_t i;
 	uint64_t vva, len;
 
+	if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM)) {
+		RTE_LOG(ERR, VHOST_CONFIG,
+			"IOMMU feature is not enabled for this dev(%s)\n",
+			dev->ifname);
+		return -1;
+	}
+
 	switch (imsg->type) {
 	case VHOST_IOTLB_UPDATE:
 		len = imsg->size;
-- 
1.9.1