diff mbox series

vhost: initializing iotlb memory only when IOMMU feature is enabled.

Message ID 1534537189-26763-1-git-send-email-nitin.katiyar@ericsson.com (mailing list archive)
State Changes Requested, archived
Delegated to: Maxime Coquelin
Headers
Series vhost: initializing iotlb memory only when IOMMU feature is enabled. |

Checks

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

Commit Message

Nitin Katiyar Aug. 17, 2018, 8:19 p.m. UTC 
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(+)

Comments

Ilya Maximets Aug. 17, 2018, 2:06 p.m. UTC | #1 
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;
>
Nitin Katiyar Aug. 23, 2018, 7:33 a.m. UTC | #2 
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;
>
diff mbox series

Patch

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;