Re: [dpdk-dev] [PATCH] examples/vhost: add error propagation in ioat ops

On 1/31/21 1:36 PM, Cheng Jiang wrote:

This change from uint16_t to int does not seem related to the purpose of
this patch. Is it?

Hi,

-----Original Message-----
From: Maxime Coquelin <redacted>
Sent: Wednesday, February 3, 2021 5:49 PM
To: Jiang, Cheng1 <redacted>; Xia, Chenbo
[off-list ref]
Cc: dev@dpdk.org; Hu, Jiayu <redacted>; Yang, YvonneX
[off-list ref]; Wang, Yinan [off-list ref]
Subject: Re: [PATCH] examples/vhost: add error propagation in ioat ops



On 1/31/21 1:36 PM, Cheng Jiang wrote:

quoted

It makes more sense to add error propagation for rte_ioat_completed_ops.

Signed-off-by: Cheng Jiang <redacted>
---
 examples/vhost/ioat.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/examples/vhost/ioat.c b/examples/vhost/ioat.c index

dbad28d43..60b73be93 100644

--- a/examples/vhost/ioat.c
+++ b/examples/vhost/ioat.c

@@ -22,7 +22,6 @@ struct packet_tracker {

 struct packet_tracker cb_tracker[MAX_VHOST_DEVICE];

-
 int
 open_ioat(const char *value)
 {

@@ -129,7 +128,7 @@ ioat_transfer_data_cb(int vid, uint16_t queue_id,
 		struct rte_vhost_async_status *opaque_data, uint16_t count)

{

quoted

 	uint32_t i_desc;
-	int dev_id = dma_bind[vid].dmas[queue_id * 2 +

VIRTIO_RXQ].dev_id;

quoted

+	uint16_t dev_id = dma_bind[vid].dmas[queue_id * 2 +
+VIRTIO_RXQ].dev_id;

This change from uint16_t to int does not seem related to the purpose of this
patch. Is it?

Yes, this change has nothing to do with adding the log, I just noticed uint16_t is more appropriate and changed here.
I can only add the log if you think change the type of dev_id here is not ok.

Thanks.
Cheng

quoted

 	struct rte_vhost_iov_iter *src = NULL;
 	struct rte_vhost_iov_iter *dst = NULL;
 	unsigned long i_seg;

@@ -182,10 +181,17 @@ ioat_check_completed_copies_cb(int vid,

uint16_t queue_id,

quoted

 		unsigned short mask = MAX_ENQUEUED_SIZE - 1;
 		unsigned short i;

-		int dev_id = dma_bind[vid].dmas[queue_id * 2
+		uint16_t dev_id = dma_bind[vid].dmas[queue_id * 2
 				+ VIRTIO_RXQ].dev_id;
 		n_seg = rte_ioat_completed_ops(dev_id, 255, dump, dump);
-		if (n_seg <= 0)
+		if (n_seg < 0) {
+			RTE_LOG(ERR,
+				VHOST_DATA,
+				"fail to poll completed buf on IOAT

device %u",

quoted

+				dev_id);
+			return 0;
+		}
+		if (n_seg == 0)
 			return 0;

 		cb_tracker[dev_id].ioat_space += n_seg;

On 2/3/21 12:52 PM, Jiang, Cheng1 wrote:

Hi,

quoted

-----Original Message-----
From: Maxime Coquelin <redacted>
Sent: Wednesday, February 3, 2021 5:49 PM
To: Jiang, Cheng1 <redacted>; Xia, Chenbo
[off-list ref]
Cc: dev@dpdk.org; Hu, Jiayu <redacted>; Yang, YvonneX
[off-list ref]; Wang, Yinan [off-list ref]
Subject: Re: [PATCH] examples/vhost: add error propagation in ioat ops



On 1/31/21 1:36 PM, Cheng Jiang wrote:

quoted

It makes more sense to add error propagation for rte_ioat_completed_ops.

Signed-off-by: Cheng Jiang <redacted>
---
 examples/vhost/ioat.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/examples/vhost/ioat.c b/examples/vhost/ioat.c index

dbad28d43..60b73be93 100644

--- a/examples/vhost/ioat.c
+++ b/examples/vhost/ioat.c

@@ -22,7 +22,6 @@ struct packet_tracker {

 struct packet_tracker cb_tracker[MAX_VHOST_DEVICE];

-
 int
 open_ioat(const char *value)
 {

@@ -129,7 +128,7 @@ ioat_transfer_data_cb(int vid, uint16_t queue_id,
 		struct rte_vhost_async_status *opaque_data, uint16_t count)

{

quoted

 	uint32_t i_desc;
-	int dev_id = dma_bind[vid].dmas[queue_id * 2 +

VIRTIO_RXQ].dev_id;

quoted

+	uint16_t dev_id = dma_bind[vid].dmas[queue_id * 2 +
+VIRTIO_RXQ].dev_id;

This change from uint16_t to int does not seem related to the purpose of this
patch. Is it?

Yes, this change has nothing to do with adding the log, I just noticed uint16_t is more appropriate and changed here.
I can only add the log if you think change the type of dev_id here is not ok.

Better to have two commits since these are two different changes.
At least please mention it in the commit message.

Thanks,
Maxime

Thanks.
Cheng

quoted

quoted

 	struct rte_vhost_iov_iter *src = NULL;
 	struct rte_vhost_iov_iter *dst = NULL;
 	unsigned long i_seg;

@@ -182,10 +181,17 @@ ioat_check_completed_copies_cb(int vid,

uint16_t queue_id,

quoted

 		unsigned short mask = MAX_ENQUEUED_SIZE - 1;
 		unsigned short i;

-		int dev_id = dma_bind[vid].dmas[queue_id * 2
+		uint16_t dev_id = dma_bind[vid].dmas[queue_id * 2
 				+ VIRTIO_RXQ].dev_id;
 		n_seg = rte_ioat_completed_ops(dev_id, 255, dump, dump);
-		if (n_seg <= 0)
+		if (n_seg < 0) {
+			RTE_LOG(ERR,
+				VHOST_DATA,
+				"fail to poll completed buf on IOAT

device %u",

quoted

+				dev_id);
+			return 0;
+		}
+		if (n_seg == 0)
 			return 0;

 		cb_tracker[dev_id].ioat_space += n_seg;

-----Original Message-----
From: Maxime Coquelin <redacted>
Sent: Wednesday, February 3, 2021 8:49 PM
To: Jiang, Cheng1 <redacted>; Xia, Chenbo
[off-list ref]
Cc: dev@dpdk.org; Hu, Jiayu <redacted>; Yang, YvonneX
[off-list ref]; Wang, Yinan [off-list ref]
Subject: Re: [PATCH] examples/vhost: add error propagation in ioat ops



On 2/3/21 12:52 PM, Jiang, Cheng1 wrote:

quoted

Hi,

quoted

-----Original Message-----
From: Maxime Coquelin <redacted>
Sent: Wednesday, February 3, 2021 5:49 PM
To: Jiang, Cheng1 <redacted>; Xia, Chenbo
[off-list ref]
Cc: dev@dpdk.org; Hu, Jiayu <redacted>; Yang, YvonneX
[off-list ref]; Wang, Yinan [off-list ref]
Subject: Re: [PATCH] examples/vhost: add error propagation in ioat
ops



On 1/31/21 1:36 PM, Cheng Jiang wrote:

quoted

It makes more sense to add error propagation for

rte_ioat_completed_ops.

quoted

quoted

quoted

Signed-off-by: Cheng Jiang <redacted>
---
 examples/vhost/ioat.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/examples/vhost/ioat.c b/examples/vhost/ioat.c index

dbad28d43..60b73be93 100644

--- a/examples/vhost/ioat.c
+++ b/examples/vhost/ioat.c

@@ -22,7 +22,6 @@ struct packet_tracker {

 struct packet_tracker cb_tracker[MAX_VHOST_DEVICE];

-
 int
 open_ioat(const char *value)
 {

@@ -129,7 +128,7 @@ ioat_transfer_data_cb(int vid, uint16_t queue_id,
 		struct rte_vhost_async_status *opaque_data, uint16_t count)

{

quoted

 	uint32_t i_desc;
-	int dev_id = dma_bind[vid].dmas[queue_id * 2 +

VIRTIO_RXQ].dev_id;

quoted

+	uint16_t dev_id = dma_bind[vid].dmas[queue_id * 2 +
+VIRTIO_RXQ].dev_id;

This change from uint16_t to int does not seem related to the purpose
of this patch. Is it?

Yes, this change has nothing to do with adding the log, I just noticed

uint16_t is more appropriate and changed here.

quoted

I can only add the log if you think change the type of dev_id here is not ok.

Better to have two commits since these are two different changes.
At least please mention it in the commit message.

Sure, I'll mention it in the commit message.

Thanks,
Cheng

Thanks,
Maxime

quoted

Thanks.
Cheng

quoted

quoted

 	struct rte_vhost_iov_iter *src = NULL;
 	struct rte_vhost_iov_iter *dst = NULL;
 	unsigned long i_seg;

@@ -182,10 +181,17 @@ ioat_check_completed_copies_cb(int vid,

uint16_t queue_id,

quoted

 		unsigned short mask = MAX_ENQUEUED_SIZE - 1;
 		unsigned short i;

-		int dev_id = dma_bind[vid].dmas[queue_id * 2
+		uint16_t dev_id = dma_bind[vid].dmas[queue_id * 2
 				+ VIRTIO_RXQ].dev_id;
 		n_seg = rte_ioat_completed_ops(dev_id, 255, dump, dump);
-		if (n_seg <= 0)
+		if (n_seg < 0) {
+			RTE_LOG(ERR,
+				VHOST_DATA,
+				"fail to poll completed buf on IOAT

device %u",

quoted

+				dev_id);
+			return 0;
+		}
+		if (n_seg == 0)
 			return 0;

 		cb_tracker[dev_id].ioat_space += n_seg;