[dpdk-dev] [RFC] ethdev: add a field for rte_eth_rxq_info

[dpdk-dev] [RFC] ethdev: add a field for rte_eth_rxq_info

[Chengchang Tang]

In common practice, PMD configure the rx_buf_size according to the data room
size of the object in mempool. But in fact the final value is related to the
specifications of hw, and its values will affect the number of fragments in
recieving pkts.

At present, we seem to have no way to espose relevant information to upper
layer users.

Add a field named rx_bufsize in rte_eth_rxq_info to indicate the buffer size
used in recieving pkts for hw.

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
---
 lib/librte_ethdev/rte_ethdev.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 0f6d053..82b7e98 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1306,6 +1306,7 @@ struct rte_eth_rxq_info {
 	struct rte_eth_rxconf conf; /**< queue config parameters. */
 	uint8_t scattered_rx;       /**< scattered packets RX supported. */
 	uint16_t nb_desc;           /**< configured number of RXDs. */
+	uint16_t rx_bufsize;        /**< size of RX buffer. */
 } __rte_cache_min_aligned;

 /**
--
2.7.4

[dpdk-dev] [PATCH 0/3] add RX buffer size for rte_eth_rxq_info

[Chengchang Tang]

In common practice, PMD configure the RX buffer size which indicate the
buffer length could be used for HW in receiving packets according to the
data room size of the object in mempool. But in fact, the final value is
related to the specifications of HW, and its values will affect the number
of fragments in receiving packets when scatter is enabled. By the way,
some PMDs may force to enable scatter when the MTU is bigger than the HW
RX buffer size.

At present, we have no way to expose relevant information to upper layer
users. So, add a field named rx_buf_size in rte_eth_rxq_info to indicate
the buffer size used in receiving packets for HW. And this field is
optional, so there is no need to forcibly update all PMDs.

This patchset also add hns3 PMD implementation and update the testpmd.

Chengchang Tang (3):
  ethdev: add a field for rte_eth_rxq_info
  app/testpmd: Add RX buffer size display in queue info querry
  net/hns3: add rx buffer size to rx qinfo querry

 app/test-pmd/config.c          | 1 +
 drivers/net/hns3/hns3_rxtx.c   | 2 ++
 lib/librte_ethdev/rte_ethdev.h | 2 ++
 3 files changed, 5 insertions(+)

--
2.7.4

[dpdk-dev] [PATCH 1/3] ethdev: add a field for rte_eth_rxq_info

[Chengchang Tang]

Add a field named rx_buf_size in rte_eth_rxq_info to indicate the buffer
size used in receiving packets for HW.

In this way, upper-layer users can get this information by calling
rte_eth_rx_queue_info_get.

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 lib/librte_ethdev/rte_ethdev.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 70295d7..9fed5cb 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1420,6 +1420,8 @@ struct rte_eth_rxq_info {
 	struct rte_eth_rxconf conf; /**< queue config parameters. */
 	uint8_t scattered_rx;       /**< scattered packets RX supported. */
 	uint16_t nb_desc;           /**< configured number of RXDs. */
+	/**< buffer size used for hardware when receive packets. */
+	uint16_t rx_buf_size;
 } __rte_cache_min_aligned;

 /**
--
2.7.4

[dpdk-dev] [PATCH 2/3] app/testpmd: Add RX buffer size display in queue info querry

[Chengchang Tang]

Add RX buffer size to queue info querry cmd so that the user can get the
buffer length used by HW queue for receiving packets.

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
---
 app/test-pmd/config.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 30bee33..b432ac6 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -452,6 +452,7 @@ rx_queue_infos_display(portid_t port_id, uint16_t queue_id)
 		(qinfo.conf.rx_deferred_start != 0) ? "on" : "off");
 	printf("\nRX scattered packets: %s",
 		(qinfo.scattered_rx != 0) ? "on" : "off");
+	printf("\nRX buffer size: %hu", qinfo.rx_buf_size);
 	printf("\nNumber of RXDs: %hu", qinfo.nb_desc);

 	if (rte_eth_rx_burst_mode_get(port_id, queue_id, &mode) == 0)
--
2.7.4

[dpdk-dev] [PATCH 3/3] net/hns3: add RX buffer size to rx qinfo querry

[Chengchang Tang]

Report hns3 PMD configured RX buffer size in RX qinfo querry.

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
---
 drivers/net/hns3/hns3_rxtx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
index fc1a256..50881d4 100644
--- a/drivers/net/hns3/hns3_rxtx.c
+++ b/drivers/net/hns3/hns3_rxtx.c
@@ -2830,6 +2830,8 @@ hns3_rxq_info_get(struct rte_eth_dev *dev, uint16_t queue_id,
 	qinfo->mp = rxq->mb_pool;
 	qinfo->nb_desc = rxq->nb_rx_desc;
 	qinfo->scattered_rx = dev->data->scattered_rx;
+	/* Report the HW RX buffer length to user */
+	qinfo->rx_buf_size = rxq->rx_buf_len;

 	/*
 	 * If there are no available Rx buffer descriptors, incoming packets
--
2.7.4

[dpdk-dev] [PATCH v2 0/3] add Rx buffer size for rxq info structure

[Chengchang Tang]

In common practice, PMD configure the Rx buffer size which indicate the
buffer length could be used for HW in receiving packets according to the
data room size of the object in mempool. But in fact, the final value is
related to the specifications of HW, and its values will affect the number
of fragments in receiving packets when scatter is enabled. By the way,
some PMDs may force to enable scatter when the MTU is bigger than the HW
Rx buffer size.

At present, we have no way to expose relevant information to upper layer
users. So, add a field named rx_buf_size in rte_eth_rxq_info to indicate
the buffer size used in receiving packets for HW. And this field is
optional, so there is no need to forcibly update all PMDs.

This patchset also add hns3 PMD implementation and update the testpmd.

Chengchang Tang (3):
  ethdev: add a field for rxq info structure
  app/testpmd: add Rx buffer size display in queue info query
  net/hns3: add Rx buffer size to Rx qinfo query

 app/test-pmd/config.c          | 1 +
 drivers/net/hns3/hns3_rxtx.c   | 2 ++
 lib/librte_ethdev/rte_ethdev.h | 2 ++
 3 files changed, 5 insertions(+)

--
2.7.4

[dpdk-dev] [PATCH v2 1/3] ethdev: add a field for rxq info structure

[Chengchang Tang]

Add a field named rx_buf_size in rte_eth_rxq_info to indicate the buffer
size used in receiving packets for HW.

In this way, upper-layer users can get this information by calling
rte_eth_rx_queue_info_get.

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
v1 -> v2: fix wrong headline format
---
 lib/librte_ethdev/rte_ethdev.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 70295d7..9fed5cb 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1420,6 +1420,8 @@ struct rte_eth_rxq_info {
 	struct rte_eth_rxconf conf; /**< queue config parameters. */
 	uint8_t scattered_rx;       /**< scattered packets RX supported. */
 	uint16_t nb_desc;           /**< configured number of RXDs. */
+	/**< buffer size used for hardware when receive packets. */
+	uint16_t rx_buf_size;
 } __rte_cache_min_aligned;

 /**
--
2.7.4

Re: [dpdk-dev] [PATCH v2 1/3] ethdev: add a field for rxq info structure

[Wei Hu (Xavier)]

On 2020/8/26 15:12, Chengchang Tang wrote:
> Add a field named rx_buf_size in rte_eth_rxq_info to indicate the buffer
> size used in receiving packets for HW.
>
> In this way, upper-layer users can get this information by calling
> rte_eth_rx_queue_info_get.
>
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> ---
> v1 -> v2: fix wrong headline format
> ---
>   lib/librte_ethdev/rte_ethdev.h | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 70295d7..9fed5cb 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1420,6 +1420,8 @@ struct rte_eth_rxq_info {
>   	struct rte_eth_rxconf conf; /**< queue config parameters. */
>   	uint8_t scattered_rx;       /**< scattered packets RX supported. */
>   	uint16_t nb_desc;           /**< configured number of RXDs. */
> +	/**< buffer size used for hardware when receive packets. */
> +	uint16_t rx_buf_size;
>   } __rte_cache_min_aligned;
>
>   /**
> --
> 2.7.4
>

[dpdk-dev] [PATCH v2 2/3] app/testpmd: add Rx buffer size display in queue info query

[Chengchang Tang]

Add Rx buffer size to queue info querry cmd so that the user can get the
buffer length used by HW queue for receiving packets.

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
---
v1 -> v2: fix some spelling mistake
---
 app/test-pmd/config.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 30bee33..b432ac6 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -452,6 +452,7 @@ rx_queue_infos_display(portid_t port_id, uint16_t queue_id)
 		(qinfo.conf.rx_deferred_start != 0) ? "on" : "off");
 	printf("\nRX scattered packets: %s",
 		(qinfo.scattered_rx != 0) ? "on" : "off");
+	printf("\nRX buffer size: %hu", qinfo.rx_buf_size);
 	printf("\nNumber of RXDs: %hu", qinfo.nb_desc);

 	if (rte_eth_rx_burst_mode_get(port_id, queue_id, &mode) == 0)
--
2.7.4

Re: [dpdk-dev] [PATCH v2 2/3] app/testpmd: add Rx buffer size display in queue info query

[Wei Hu (Xavier)]

On 2020/8/26 15:12, Chengchang Tang wrote:
> Add Rx buffer size to queue info querry cmd so that the user can get the
> buffer length used by HW queue for receiving packets.
>
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> ---
> v1 -> v2: fix some spelling mistake
> ---
>   app/test-pmd/config.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 30bee33..b432ac6 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -452,6 +452,7 @@ rx_queue_infos_display(portid_t port_id, uint16_t queue_id)
>   		(qinfo.conf.rx_deferred_start != 0) ? "on" : "off");
>   	printf("\nRX scattered packets: %s",
>   		(qinfo.scattered_rx != 0) ? "on" : "off");
> +	printf("\nRX buffer size: %hu", qinfo.rx_buf_size);
>   	printf("\nNumber of RXDs: %hu", qinfo.nb_desc);
>
>   	if (rte_eth_rx_burst_mode_get(port_id, queue_id, &mode) == 0)
> --
> 2.7.4
>

Re: [dpdk-dev] [PATCH v2 2/3] app/testpmd: add Rx buffer size display in queue info query

[Stephen Hemminger]

On Wed, 26 Aug 2020 15:12:22 +0800
Chengchang Tang <tangchengchang@huawei.com> wrote:

> Add Rx buffer size to queue info querry cmd so that the user can get the
> buffer length used by HW queue for receiving packets.
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>

You might want to add this info to proc-info tool as well.

Re: [dpdk-dev] [PATCH v2 2/3] app/testpmd: add Rx buffer size display in queue info query

[Chengchang Tang]

Hi, Stephen Hemminger

On 2020/8/26 22:42, Stephen Hemminger wrote:
> On Wed, 26 Aug 2020 15:12:22 +0800
> Chengchang Tang <tangchengchang@huawei.com> wrote:
> 
>> Add Rx buffer size to queue info querry cmd so that the user can get the
>> buffer length used by HW queue for receiving packets.
>>
>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> 
> You might want to add this info to proc-info tool as well.
> 
> .
> 
Thanks for your suggestion.

I will add this to proc-info tool in the next version.

Thanks, Chengchang

[dpdk-dev] [PATCH v2 3/3] net/hns3: add Rx buffer size to Rx qinfo query

[Chengchang Tang]

Report hns3 PMD configured Rx buffer size in Rx queue information query.

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
---
v1 -> v2: fix some spelling mistake.
---
 drivers/net/hns3/hns3_rxtx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
index fc1a256..50881d4 100644
--- a/drivers/net/hns3/hns3_rxtx.c
+++ b/drivers/net/hns3/hns3_rxtx.c
@@ -2830,6 +2830,8 @@ hns3_rxq_info_get(struct rte_eth_dev *dev, uint16_t queue_id,
 	qinfo->mp = rxq->mb_pool;
 	qinfo->nb_desc = rxq->nb_rx_desc;
 	qinfo->scattered_rx = dev->data->scattered_rx;
+	/* Report the HW Rx buffer length to user */
+	qinfo->rx_buf_size = rxq->rx_buf_len;

 	/*
 	 * If there are no available Rx buffer descriptors, incoming packets
--
2.7.4

Re: [dpdk-dev] [PATCH v2 3/3] net/hns3: add Rx buffer size to Rx qinfo query

[Wei Hu (Xavier)]

On 2020/8/26 15:12, Chengchang Tang wrote:
> Report hns3 PMD configured Rx buffer size in Rx queue information query.
>
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> ---
> v1 -> v2: fix some spelling mistake.
> ---
>   drivers/net/hns3/hns3_rxtx.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
> index fc1a256..50881d4 100644
> --- a/drivers/net/hns3/hns3_rxtx.c
> +++ b/drivers/net/hns3/hns3_rxtx.c
> @@ -2830,6 +2830,8 @@ hns3_rxq_info_get(struct rte_eth_dev *dev, uint16_t queue_id,
>   	qinfo->mp = rxq->mb_pool;
>   	qinfo->nb_desc = rxq->nb_rx_desc;
>   	qinfo->scattered_rx = dev->data->scattered_rx;
> +	/* Report the HW Rx buffer length to user */
> +	qinfo->rx_buf_size = rxq->rx_buf_len;
>
>   	/*
>   	 * If there are no available Rx buffer descriptors, incoming packets
> --
> 2.7.4
>

[dpdk-dev] [PATCH v3 0/4] add Rx buffer size for rxq info structure

[Chengchang Tang]

In common practice, PMD configure the Rx buffer size which indicate the
buffer length could be used for HW in receiving packets according to the
data room size of the object in mempool. But in fact, the final value is
related to the specifications of HW, and its values will affect the number
of fragments in receiving packets when scatter is enabled. By the way,
some PMDs may force to enable scatter when the MTU is bigger than the HW
Rx buffer size.

At present, we have no way to expose relevant information to upper layer
users. So, add a field named rx_buf_size in rte_eth_rxq_info to indicate
the buffer size used in receiving packets for HW. And this field is
optional, so there is no need to forcibly update all PMDs.

This patchset also update testpmd, proc-info tools and add hns3 PMD
implementation.

Chengchang Tang (4):
  ethdev: add a field for rxq info structure
  app/testpmd: add Rx buffer size display in queue info query
  app/procinfo: add Rx buffer size to --show-port
  net/hns3: add Rx buffer size to Rx qinfo query

 app/proc-info/main.c           | 2 ++
 app/test-pmd/config.c          | 1 +
 drivers/net/hns3/hns3_rxtx.c   | 2 ++
 lib/librte_ethdev/rte_ethdev.h | 2 ++
 4 files changed, 7 insertions(+)

--
2.7.4

[dpdk-dev] [PATCH v3 1/4] ethdev: add a field for rxq info structure

[Chengchang Tang]

Add a field named rx_buf_size in rte_eth_rxq_info to indicate the buffer
size used in receiving packets for HW.

In this way, upper-layer users can get this information by calling
rte_eth_rx_queue_info_get.

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 lib/librte_ethdev/rte_ethdev.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 70295d7..9fed5cb 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1420,6 +1420,8 @@ struct rte_eth_rxq_info {
 	struct rte_eth_rxconf conf; /**< queue config parameters. */
 	uint8_t scattered_rx;       /**< scattered packets RX supported. */
 	uint16_t nb_desc;           /**< configured number of RXDs. */
+	/**< buffer size used for hardware when receive packets. */
+	uint16_t rx_buf_size;
 } __rte_cache_min_aligned;

 /**
--
2.7.4

Re: [dpdk-dev] [PATCH v3 1/4] ethdev: add a field for rxq info structure

[Matan Azrad]

Hi Chengchang

Please see some question below.

From: Chengchang Tang
> Add a field named rx_buf_size in rte_eth_rxq_info to indicate the buffer size
> used in receiving packets for HW.
> 
> In this way, upper-layer users can get this information by calling
> rte_eth_rx_queue_info_get.
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
>  lib/librte_ethdev/rte_ethdev.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 70295d7..9fed5cb 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1420,6 +1420,8 @@ struct rte_eth_rxq_info {
>         struct rte_eth_rxconf conf; /**< queue config parameters. */
>         uint8_t scattered_rx;       /**< scattered packets RX supported. */
>         uint16_t nb_desc;           /**< configured number of RXDs. */
> +       /**< buffer size used for hardware when receive packets. */
> +       uint16_t rx_buf_size;

Is it the maximum supported Rx buffer by the HW?
If yes, maybe max_rx_buf_size is better name?

Maybe document that 0 means - no limitation by HW?

Must application read it in order to know if its datapath should handle multi-segment buffers?

Maybe it will be good to force application to configure scatter when this field is valid and smaller than max_rx_pkt_len\max_lro.. (<= room size)...


>  } __rte_cache_min_aligned;
> 
>  /**
> --
> 2.7.4

Re: [dpdk-dev] [PATCH v3 1/4] ethdev: add a field for rxq info structure

[Chengchang Tang]

Hi, Matan

On 2020/9/1 23:33, Matan Azrad wrote:
> 
> Hi Chengchang
> 
> Please see some question below.
> 
> From: Chengchang Tang
>> Add a field named rx_buf_size in rte_eth_rxq_info to indicate the buffer size
>> used in receiving packets for HW.
>>
>> In this way, upper-layer users can get this information by calling
>> rte_eth_rx_queue_info_get.
>>
>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
>>  lib/librte_ethdev/rte_ethdev.h | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>> index 70295d7..9fed5cb 100644
>> --- a/lib/librte_ethdev/rte_ethdev.h
>> +++ b/lib/librte_ethdev/rte_ethdev.h
>> @@ -1420,6 +1420,8 @@ struct rte_eth_rxq_info {
>>         struct rte_eth_rxconf conf; /**< queue config parameters. */
>>         uint8_t scattered_rx;       /**< scattered packets RX supported. */
>>         uint16_t nb_desc;           /**< configured number of RXDs. */
>> +       /**< buffer size used for hardware when receive packets. */
>> +       uint16_t rx_buf_size;
> 
> Is it the maximum supported Rx buffer by the HW?
> If yes, maybe max_rx_buf_size is better name?

No, it is the Rx buffer size currently used by HW.

IMHO, the structure rte_eth_rxq_info and associated query API are mainly used to query HW configurations
at runtime or after queue is configured/setup. Therefore, the content of this structure should be the
current HW configuration.

> Maybe document that 0 means - no limitation by HW?

Yes, there is no need to fill this filed for HW that has no restrictions on it.
I'll add it in v4.

> Must application read it in order to know if its datapath should handle multi-segment buffers?

I think it's more appropriate to use scattered_rx to determine if multi-segment buffers should be handled.

> 
> Maybe it will be good to force application to configure scatter when this field is valid and smaller than max_rx_pkt_len\max_lro.. (<= room size)...
> 
> 
>>  } __rte_cache_min_aligned;
>>
>>  /**

Re: [dpdk-dev] [PATCH v3 1/4] ethdev: add a field for rxq info structure

[Matan Azrad]

Hi Chengchang

From: Chengchang Tang
> Hi, Matan
> 
> On 2020/9/1 23:33, Matan Azrad wrote:
> >
> > Hi Chengchang
> >
> > Please see some question below.
> >
> > From: Chengchang Tang
> >> Add a field named rx_buf_size in rte_eth_rxq_info to indicate the
> >> buffer size used in receiving packets for HW.
> >>
> >> In this way, upper-layer users can get this information by calling
> >> rte_eth_rx_queue_info_get.
> >>
> >> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> >> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> >> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> >> ---
> >>  lib/librte_ethdev/rte_ethdev.h | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/lib/librte_ethdev/rte_ethdev.h
> >> b/lib/librte_ethdev/rte_ethdev.h index 70295d7..9fed5cb 100644
> >> --- a/lib/librte_ethdev/rte_ethdev.h
> >> +++ b/lib/librte_ethdev/rte_ethdev.h
> >> @@ -1420,6 +1420,8 @@ struct rte_eth_rxq_info {
> >>         struct rte_eth_rxconf conf; /**< queue config parameters. */
> >>         uint8_t scattered_rx;       /**< scattered packets RX supported. */
> >>         uint16_t nb_desc;           /**< configured number of RXDs. */
> >> +       /**< buffer size used for hardware when receive packets. */
> >> +       uint16_t rx_buf_size;
> >
> > Is it the maximum supported Rx buffer by the HW?
> > If yes, maybe max_rx_buf_size is better name?
> 
> No, it is the Rx buffer size currently used by HW.

Doesn't it defined by the user? Using Rx queue mem-pool mbuf room size?

And it may be different per Rx queue....

> IMHO, the structure rte_eth_rxq_info and associated query API are mainly
> used to query HW configurations at runtime or after queue is
> configured/setup. Therefore, the content of this structure should be the
> current HW configuration.

It looks me more like capabilities...
The one which define the current configuration is the user by the configuration APIs(after reading the capabilities).

I don't think we have here all the current configurations, so what is special in this one?


> > Maybe document that 0 means - no limitation by HW?
> 
> Yes, there is no need to fill this filed for HW that has no restrictions on it.
> I'll add it in v4.
> 
> > Must application read it in order to know if its datapath should handle
> multi-segment buffers?
> 
> I think it's more appropriate to use scattered_rx to determine if multi-
> segment buffers should be handled.
> 
> >
> > Maybe it will be good to force application to configure scatter when this
> field is valid and smaller than max_rx_pkt_len\max_lro.. (<= room size)...

Can you explain more what is the issue you came to solve?

> >
> >>  } __rte_cache_min_aligned;
> >>
> >>  /**
> 

Re: [dpdk-dev] [PATCH v3 1/4] ethdev: add a field for rxq info structure

[Chengchang Tang]

Hi, Matan

On 2020/9/2 15:19, Matan Azrad wrote:
> 
> Hi Chengchang
> 
> From: Chengchang Tang
>> Hi, Matan
>>
>> On 2020/9/1 23:33, Matan Azrad wrote:
>>>
>>> Hi Chengchang
>>>
>>> Please see some question below.
>>>
>>> From: Chengchang Tang
>>>> Add a field named rx_buf_size in rte_eth_rxq_info to indicate the
>>>> buffer size used in receiving packets for HW.
>>>>
>>>> In this way, upper-layer users can get this information by calling
>>>> rte_eth_rx_queue_info_get.
>>>>
>>>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>>>> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>>>> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>>> ---
>>>>  lib/librte_ethdev/rte_ethdev.h | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/lib/librte_ethdev/rte_ethdev.h
>>>> b/lib/librte_ethdev/rte_ethdev.h index 70295d7..9fed5cb 100644
>>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>>> @@ -1420,6 +1420,8 @@ struct rte_eth_rxq_info {
>>>>         struct rte_eth_rxconf conf; /**< queue config parameters. */
>>>>         uint8_t scattered_rx;       /**< scattered packets RX supported. */
>>>>         uint16_t nb_desc;           /**< configured number of RXDs. */
>>>> +       /**< buffer size used for hardware when receive packets. */
>>>> +       uint16_t rx_buf_size;
>>>
>>> Is it the maximum supported Rx buffer by the HW?
>>> If yes, maybe max_rx_buf_size is better name?
>>
>> No, it is the Rx buffer size currently used by HW.
> 
> Doesn't it defined by the user? Using Rx queue mem-pool mbuf room size?
>
> And it may be different per Rx queue....

Yes, it is defined by user using the Rx queue mem-pool mbuf room size.
When different queues are bound to different mempools, different queues may have different value.
> 
>> IMHO, the structure rte_eth_rxq_info and associated query API are mainly
>> used to query HW configurations at runtime or after queue is
>> configured/setup. Therefore, the content of this structure should be the
>> current HW configuration.
> 
> It looks me more like capabilities...
> The one which define the current configuration is the user by the configuration APIs(after reading the capabilities).

I prefer to consider the structure rte_eth_dev_info as the capabilities. Because rxq_info and
associated APIs are not available until the queue is configured. And the max rx_buf_size is
already exists in dev_info.
>
> I don't think we have here all the current configurations, so what is special in this one?

I think the structure is used to store the queue-related configuration, especially the final HW
configuration that may be different from user configuration and some configurations that are not
mandatory for the user(PMDs will use a default configuration). Such as the scatterred_rx and
rx_drop_en in rte_eth_rxconf, some PMDs will adjust it in some cases based on their HW constraints.

This configuration item meets the above criteria. The value range of rx_buf_size varies according
to HW. Some HW may require 1k-alignment, while others may require several fixed values. So, the PMDs
will configure it based on their HW constraints. This results in difference between the user
configuration and the actual configuration and this value affects the memory usage and performance.
I think there's a need for a way to expose that information.
> 
> 
>>> Maybe document that 0 means - no limitation by HW?
>>
>> Yes, there is no need to fill this filed for HW that has no restrictions on it.
>> I'll add it in v4.
>>
>>> Must application read it in order to know if its datapath should handle
>> multi-segment buffers?
>>
>> I think it's more appropriate to use scattered_rx to determine if multi-
>> segment buffers should be handled.
>>
>>>
>>> Maybe it will be good to force application to configure scatter when this
>> field is valid and smaller than max_rx_pkt_len\max_lro.. (<= room size)...
> 
> Can you explain more what is the issue you came to solve?

This HW information may be useful when there is some problems running a application. This structure
and related APIs can be used to expose it at run time.
> 
>>>
>>>>  } __rte_cache_min_aligned;
>>>>
>>>>  /**
>>
> .
> 

Re: [dpdk-dev] [PATCH v3 1/4] ethdev: add a field for rxq info structure

[Matan Azrad]

Hi Chengchang

From: Chengchang Tang
> Hi, Matan
> 
> On 2020/9/2 15:19, Matan Azrad wrote:
> >
> > Hi Chengchang
> >
> > From: Chengchang Tang
> >> Hi, Matan
> >>
> >> On 2020/9/1 23:33, Matan Azrad wrote:
> >>>
> >>> Hi Chengchang
> >>>
> >>> Please see some question below.
> >>>
> >>> From: Chengchang Tang
> >>>> Add a field named rx_buf_size in rte_eth_rxq_info to indicate the
> >>>> buffer size used in receiving packets for HW.
> >>>>
> >>>> In this way, upper-layer users can get this information by calling
> >>>> rte_eth_rx_queue_info_get.
> >>>>
> >>>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> >>>> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> >>>> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> >>>> ---
> >>>>  lib/librte_ethdev/rte_ethdev.h | 2 ++
> >>>>  1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/lib/librte_ethdev/rte_ethdev.h
> >>>> b/lib/librte_ethdev/rte_ethdev.h index 70295d7..9fed5cb 100644
> >>>> --- a/lib/librte_ethdev/rte_ethdev.h
> >>>> +++ b/lib/librte_ethdev/rte_ethdev.h
> >>>> @@ -1420,6 +1420,8 @@ struct rte_eth_rxq_info {
> >>>>         struct rte_eth_rxconf conf; /**< queue config parameters. */
> >>>>         uint8_t scattered_rx;       /**< scattered packets RX supported. */
> >>>>         uint16_t nb_desc;           /**< configured number of RXDs. */
> >>>> +       /**< buffer size used for hardware when receive packets. */
> >>>> +       uint16_t rx_buf_size;
> >>>
> >>> Is it the maximum supported Rx buffer by the HW?
> >>> If yes, maybe max_rx_buf_size is better name?
> >>
> >> No, it is the Rx buffer size currently used by HW.

> > Doesn't it defined by the user? Using Rx queue mem-pool mbuf room size?
> >
> > And it may be different per Rx queue....
> 
> Yes, it is defined by user using the Rx queue mem-pool mbuf room size.
> When different queues are bound to different mempools, different queues
> may have different value.
> >
> >> IMHO, the structure rte_eth_rxq_info and associated query API are
> >> mainly used to query HW configurations at runtime or after queue is
> >> configured/setup. Therefore, the content of this structure should be
> >> the current HW configuration.
> >
> > It looks me more like capabilities...
> > The one which define the current configuration is the user by the
> configuration APIs(after reading the capabilities).
> 
> I prefer to consider the structure rte_eth_dev_info as the capabilities.

Yes.

> Because rxq_info and associated APIs are not available until the queue is
> configured. And the max rx_buf_size is already exists in dev_info.
> >
> > I don't think we have here all the current configurations, so what is special
> in this one?
> 
> I think the structure is used to store the queue-related configuration,
> especially the final HW configuration that may be different from user
> configuration and some configurations that are not mandatory for the
> user(PMDs will use a default configuration). Such as the scatterred_rx and
> rx_drop_en in rte_eth_rxconf, some PMDs will adjust it in some cases based
> on their HW constraints.

Ok, this struct(struct rte_eth_rxq_info) is new for me.
Thanks for the explanation.
 
> This configuration item meets the above criteria. The value range of
> rx_buf_size varies according to HW. Some HW may require 1k-alignment,
> while others may require several fixed values. So, the PMDs will configure it
> based on their HW constraints. This results in difference between the user
> configuration and the actual configuration and this value affects the memory
> usage and performance.
> I think there's a need for a way to expose that information.

So the user can configure X and the driver will use Y!=X?

Should the application validate its own configurations after setting them successfully?

> >
> >>> Maybe document that 0 means - no limitation by HW?
> >>
> >> Yes, there is no need to fill this filed for HW that has no restrictions on it.
> >> I'll add it in v4.
> >>
> >>> Must application read it in order to know if its datapath should
> >>> handle
> >> multi-segment buffers?
> >>
> >> I think it's more appropriate to use scattered_rx to determine if
> >> multi- segment buffers should be handled.
> >>
> >>>
> >>> Maybe it will be good to force application to configure scatter when
> >>> this
> >> field is valid and smaller than max_rx_pkt_len\max_lro.. (<= room size)...
> >
> > Can you explain more what is the issue you came to solve?
> 
> This HW information may be useful when there is some problems running a
> application. This structure and related APIs can be used to expose it at run
> time.
> >
OK

Re: [dpdk-dev] [PATCH v3 1/4] ethdev: add a field for rxq info structure

[Chengchang Tang]

Hi, Matan

On 2020/9/2 18:30, Matan Azrad wrote:
> Hi Chengchang
> 
> From: Chengchang Tang
>> Hi, Matan
>>
>> On 2020/9/2 15:19, Matan Azrad wrote:
>>>
>>> Hi Chengchang
>>>
>>> From: Chengchang Tang
>>>> Hi, Matan
>>>>
>>>> On 2020/9/1 23:33, Matan Azrad wrote:
>>>>>
>>>>> Hi Chengchang
>>>>>
>>>>> Please see some question below.
>>>>>
>>>>> From: Chengchang Tang
>>>>>> Add a field named rx_buf_size in rte_eth_rxq_info to indicate the
>>>>>> buffer size used in receiving packets for HW.
>>>>>>
>>>>>> In this way, upper-layer users can get this information by calling
>>>>>> rte_eth_rx_queue_info_get.
>>>>>>
>>>>>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>>>>>> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>>>>>> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>>>>> ---
>>>>>>  lib/librte_ethdev/rte_ethdev.h | 2 ++
>>>>>>  1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/lib/librte_ethdev/rte_ethdev.h
>>>>>> b/lib/librte_ethdev/rte_ethdev.h index 70295d7..9fed5cb 100644
>>>>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>>>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>>>>> @@ -1420,6 +1420,8 @@ struct rte_eth_rxq_info {
>>>>>>         struct rte_eth_rxconf conf; /**< queue config parameters. */
>>>>>>         uint8_t scattered_rx;       /**< scattered packets RX supported. */
>>>>>>         uint16_t nb_desc;           /**< configured number of RXDs. */
>>>>>> +       /**< buffer size used for hardware when receive packets. */
>>>>>> +       uint16_t rx_buf_size;
>>>>>
>>>>> Is it the maximum supported Rx buffer by the HW?
>>>>> If yes, maybe max_rx_buf_size is better name?
>>>>
>>>> No, it is the Rx buffer size currently used by HW.
> 
>>> Doesn't it defined by the user? Using Rx queue mem-pool mbuf room size?
>>>
>>> And it may be different per Rx queue....
>>
>> Yes, it is defined by user using the Rx queue mem-pool mbuf room size.
>> When different queues are bound to different mempools, different queues
>> may have different value.
>>>
>>>> IMHO, the structure rte_eth_rxq_info and associated query API are
>>>> mainly used to query HW configurations at runtime or after queue is
>>>> configured/setup. Therefore, the content of this structure should be
>>>> the current HW configuration.
>>>
>>> It looks me more like capabilities...
>>> The one which define the current configuration is the user by the
>> configuration APIs(after reading the capabilities).
>>
>> I prefer to consider the structure rte_eth_dev_info as the capabilities.
> 
> Yes.
> 
>> Because rxq_info and associated APIs are not available until the queue is
>> configured. And the max rx_buf_size is already exists in dev_info.
>>>
>>> I don't think we have here all the current configurations, so what is special
>> in this one?
>>
>> I think the structure is used to store the queue-related configuration,
>> especially the final HW configuration that may be different from user
>> configuration and some configurations that are not mandatory for the
>> user(PMDs will use a default configuration). Such as the scatterred_rx and
>> rx_drop_en in rte_eth_rxconf, some PMDs will adjust it in some cases based
>> on their HW constraints.
> 
> Ok, this struct(struct rte_eth_rxq_info) is new for me.
> Thanks for the explanation.
>  
>> This configuration item meets the above criteria. The value range of
>> rx_buf_size varies according to HW. Some HW may require 1k-alignment,
>> while others may require several fixed values. So, the PMDs will configure it
>> based on their HW constraints. This results in difference between the user
>> configuration and the actual configuration and this value affects the memory
>> usage and performance.
>> I think there's a need for a way to expose that information.
> 
> So the user can configure X and the driver will use Y!=X?

Yes, it depends on the HW. In the queue setup API, it just checks whether the input is greater
than the required minimum value. But HW usually has requirements for alignment and so on.
So when X does not meet these requirements, PMDs will calculate a new value Y that meets these
requirements to configure the hardware (Y <= X, to ensure no memory overflow occurs).
> 
> Should the application validate its own configurations after setting them successfully?

It depends on their own needs. The application should not be forced to verify it to avoid affecting
the ease of use of PMDs. For some applications, they don't care about this value.
> 
>>>
>>>>> Maybe document that 0 means - no limitation by HW?
>>>>
>>>> Yes, there is no need to fill this filed for HW that has no restrictions on it.
>>>> I'll add it in v4.
>>>>
>>>>> Must application read it in order to know if its datapath should
>>>>> handle
>>>> multi-segment buffers?
>>>>
>>>> I think it's more appropriate to use scattered_rx to determine if
>>>> multi- segment buffers should be handled.
>>>>
>>>>>
>>>>> Maybe it will be good to force application to configure scatter when
>>>>> this
>>>> field is valid and smaller than max_rx_pkt_len\max_lro.. (<= room size)...
>>>
>>> Can you explain more what is the issue you came to solve?
>>
>> This HW information may be useful when there is some problems running a
>> application. This structure and related APIs can be used to expose it at run
>> time.
>>>
> OK
> 
> .
> 

Re: [dpdk-dev] [PATCH v3 1/4] ethdev: add a field for rxq info structure

[Matan Azrad]

Hi  Chengchang

From: Chengchang Tang:
> Hi, Matan
> 
> On 2020/9/2 18:30, Matan Azrad wrote:
> > Hi Chengchang
> >
> > From: Chengchang Tang
> >> Hi, Matan
> >>
> >> On 2020/9/2 15:19, Matan Azrad wrote:
> >>>
> >>> Hi Chengchang
> >>>
> >>> From: Chengchang Tang
> >>>> Hi, Matan
> >>>>
> >>>> On 2020/9/1 23:33, Matan Azrad wrote:
> >>>>>
> >>>>> Hi Chengchang
> >>>>>
> >>>>> Please see some question below.
> >>>>>
> >>>>> From: Chengchang Tang
> >>>>>> Add a field named rx_buf_size in rte_eth_rxq_info to indicate the
> >>>>>> buffer size used in receiving packets for HW.
> >>>>>>
> >>>>>> In this way, upper-layer users can get this information by
> >>>>>> calling rte_eth_rx_queue_info_get.
> >>>>>>
> >>>>>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> >>>>>> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> >>>>>> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> >>>>>> ---
> >>>>>>  lib/librte_ethdev/rte_ethdev.h | 2 ++
> >>>>>>  1 file changed, 2 insertions(+)
> >>>>>>
> >>>>>> diff --git a/lib/librte_ethdev/rte_ethdev.h
> >>>>>> b/lib/librte_ethdev/rte_ethdev.h index 70295d7..9fed5cb 100644
> >>>>>> --- a/lib/librte_ethdev/rte_ethdev.h
> >>>>>> +++ b/lib/librte_ethdev/rte_ethdev.h
> >>>>>> @@ -1420,6 +1420,8 @@ struct rte_eth_rxq_info {
> >>>>>>         struct rte_eth_rxconf conf; /**< queue config parameters. */
> >>>>>>         uint8_t scattered_rx;       /**< scattered packets RX supported.
> */
> >>>>>>         uint16_t nb_desc;           /**< configured number of RXDs. */
> >>>>>> +       /**< buffer size used for hardware when receive packets. */
> >>>>>> +       uint16_t rx_buf_size;
> >>>>>
> >>>>> Is it the maximum supported Rx buffer by the HW?
> >>>>> If yes, maybe max_rx_buf_size is better name?
> >>>>
> >>>> No, it is the Rx buffer size currently used by HW.
> >
> >>> Doesn't it defined by the user? Using Rx queue mem-pool mbuf room
> size?
> >>>
> >>> And it may be different per Rx queue....
> >>
> >> Yes, it is defined by user using the Rx queue mem-pool mbuf room size.
> >> When different queues are bound to different mempools, different
> >> queues may have different value.
> >>>
> >>>> IMHO, the structure rte_eth_rxq_info and associated query API are
> >>>> mainly used to query HW configurations at runtime or after queue is
> >>>> configured/setup. Therefore, the content of this structure should
> >>>> be the current HW configuration.
> >>>
> >>> It looks me more like capabilities...
> >>> The one which define the current configuration is the user by the
> >> configuration APIs(after reading the capabilities).
> >>
> >> I prefer to consider the structure rte_eth_dev_info as the capabilities.
> >
> > Yes.
> >
> >> Because rxq_info and associated APIs are not available until the
> >> queue is configured. And the max rx_buf_size is already exists in
> dev_info.
> >>>
> >>> I don't think we have here all the current configurations, so what
> >>> is special
> >> in this one?
> >>
> >> I think the structure is used to store the queue-related
> >> configuration, especially the final HW configuration that may be
> >> different from user configuration and some configurations that are
> >> not mandatory for the user(PMDs will use a default configuration).
> >> Such as the scatterred_rx and rx_drop_en in rte_eth_rxconf, some PMDs
> >> will adjust it in some cases based on their HW constraints.
> >
> > Ok, this struct(struct rte_eth_rxq_info) is new for me.
> > Thanks for the explanation.
> >
> >> This configuration item meets the above criteria. The value range of
> >> rx_buf_size varies according to HW. Some HW may require 1k-alignment,
> >> while others may require several fixed values. So, the PMDs will
> >> configure it based on their HW constraints. This results in
> >> difference between the user configuration and the actual
> >> configuration and this value affects the memory usage and performance.
> >> I think there's a need for a way to expose that information.
> >
> > So the user can configure X and the driver will use Y!=X?
> 
> Yes, it depends on the HW. In the queue setup API, it just checks whether
> the input is greater than the required minimum value. But HW usually has
> requirements for alignment and so on.
> So when X does not meet these requirements, PMDs will calculate a new
> value Y that meets these requirements to configure the hardware (Y <= X, to
> ensure no memory overflow occurs).
> > Should the application validate its own configurations after setting them
> successfully?
> 
> It depends on their own needs. The application should not be forced to
> verify it to avoid affecting the ease of use of PMDs. For some applications,
> they don't care about this value.

I understand, 
It looks me like a bad ping-pong between app and PMD (for all the fields in the struct),
And we should avoid adding fields to this structure if we can.

What's about adding field in rte_eth_dev_info to expose the rx buffer alignment supported by the PMD?
Then, application has all the knowledge you want to expose before the configuration.

> >
> >>>
> >>>>> Maybe document that 0 means - no limitation by HW?
> >>>>
> >>>> Yes, there is no need to fill this filed for HW that has no restrictions on
> it.
> >>>> I'll add it in v4.
> >>>>
> >>>>> Must application read it in order to know if its datapath should
> >>>>> handle
> >>>> multi-segment buffers?
> >>>>
> >>>> I think it's more appropriate to use scattered_rx to determine if
> >>>> multi- segment buffers should be handled.
> >>>>
> >>>>>
> >>>>> Maybe it will be good to force application to configure scatter
> >>>>> when this
> >>>> field is valid and smaller than max_rx_pkt_len\max_lro.. (<= room
> size)...
> >>>
> >>> Can you explain more what is the issue you came to solve?
> >>
> >> This HW information may be useful when there is some problems running
> >> a application. This structure and related APIs can be used to expose
> >> it at run time.
> >>>
> > OK
> >
> > .
> >

Re: [dpdk-dev] [PATCH v3 1/4] ethdev: add a field for rxq info structure

[Chengchang Tang]

Hi Matan

On 2020/9/6 21:45, Matan Azrad wrote:
> 
> Hi  Chengchang
> 
> From: Chengchang Tang:
>> Hi, Matan
>>
>> On 2020/9/2 18:30, Matan Azrad wrote:
>>> Hi Chengchang
>>>
>>> From: Chengchang Tang
>>>> Hi, Matan
>>>>
>>>> On 2020/9/2 15:19, Matan Azrad wrote:
>>>>>
>>>>> Hi Chengchang
>>>>>
>>>>> From: Chengchang Tang
>>>>>> Hi, Matan
>>>>>>
>>>>>> On 2020/9/1 23:33, Matan Azrad wrote:
>>>>>>>
>>>>>>> Hi Chengchang
>>>>>>>
>>>>>>> Please see some question below.
>>>>>>>
>>>>>>> From: Chengchang Tang
>>>>>>>> Add a field named rx_buf_size in rte_eth_rxq_info to indicate the
>>>>>>>> buffer size used in receiving packets for HW.
>>>>>>>>
>>>>>>>> In this way, upper-layer users can get this information by
>>>>>>>> calling rte_eth_rx_queue_info_get.
>>>>>>>>
>>>>>>>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>>>>>>>> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>>>>>>>> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>>>>>>> ---
>>>>>>>>  lib/librte_ethdev/rte_ethdev.h | 2 ++
>>>>>>>>  1 file changed, 2 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/lib/librte_ethdev/rte_ethdev.h
>>>>>>>> b/lib/librte_ethdev/rte_ethdev.h index 70295d7..9fed5cb 100644
>>>>>>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>>>>>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>>>>>>> @@ -1420,6 +1420,8 @@ struct rte_eth_rxq_info {
>>>>>>>>         struct rte_eth_rxconf conf; /**< queue config parameters. */
>>>>>>>>         uint8_t scattered_rx;       /**< scattered packets RX supported.
>> */
>>>>>>>>         uint16_t nb_desc;           /**< configured number of RXDs. */
>>>>>>>> +       /**< buffer size used for hardware when receive packets. */
>>>>>>>> +       uint16_t rx_buf_size;
>>>>>>>
>>>>>>> Is it the maximum supported Rx buffer by the HW?
>>>>>>> If yes, maybe max_rx_buf_size is better name?
>>>>>>
>>>>>> No, it is the Rx buffer size currently used by HW.
>>>
>>>>> Doesn't it defined by the user? Using Rx queue mem-pool mbuf room
>> size?
>>>>>
>>>>> And it may be different per Rx queue....
>>>>
>>>> Yes, it is defined by user using the Rx queue mem-pool mbuf room size.
>>>> When different queues are bound to different mempools, different
>>>> queues may have different value.
>>>>>
>>>>>> IMHO, the structure rte_eth_rxq_info and associated query API are
>>>>>> mainly used to query HW configurations at runtime or after queue is
>>>>>> configured/setup. Therefore, the content of this structure should
>>>>>> be the current HW configuration.
>>>>>
>>>>> It looks me more like capabilities...
>>>>> The one which define the current configuration is the user by the
>>>> configuration APIs(after reading the capabilities).
>>>>
>>>> I prefer to consider the structure rte_eth_dev_info as the capabilities.
>>>
>>> Yes.
>>>
>>>> Because rxq_info and associated APIs are not available until the
>>>> queue is configured. And the max rx_buf_size is already exists in
>> dev_info.
>>>>>
>>>>> I don't think we have here all the current configurations, so what
>>>>> is special
>>>> in this one?
>>>>
>>>> I think the structure is used to store the queue-related
>>>> configuration, especially the final HW configuration that may be
>>>> different from user configuration and some configurations that are
>>>> not mandatory for the user(PMDs will use a default configuration).
>>>> Such as the scatterred_rx and rx_drop_en in rte_eth_rxconf, some PMDs
>>>> will adjust it in some cases based on their HW constraints.
>>>
>>> Ok, this struct(struct rte_eth_rxq_info) is new for me.
>>> Thanks for the explanation.
>>>
>>>> This configuration item meets the above criteria. The value range of
>>>> rx_buf_size varies according to HW. Some HW may require 1k-alignment,
>>>> while others may require several fixed values. So, the PMDs will
>>>> configure it based on their HW constraints. This results in
>>>> difference between the user configuration and the actual
>>>> configuration and this value affects the memory usage and performance.
>>>> I think there's a need for a way to expose that information.
>>>
>>> So the user can configure X and the driver will use Y!=X?
>>
>> Yes, it depends on the HW. In the queue setup API, it just checks whether
>> the input is greater than the required minimum value. But HW usually has
>> requirements for alignment and so on.
>> So when X does not meet these requirements, PMDs will calculate a new
>> value Y that meets these requirements to configure the hardware (Y <= X, to
>> ensure no memory overflow occurs).
>>> Should the application validate its own configurations after setting them
>> successfully?
>>
>> It depends on their own needs. The application should not be forced to
>> verify it to avoid affecting the ease of use of PMDs. For some applications,
>> they don't care about this value.
> 
> I understand, 
> It looks me like a bad ping-pong between app and PMD (for all the fields in the struct),
> And we should avoid adding fields to this structure if we can.
> 
> What's about adding field in rte_eth_dev_info to expose the rx buffer alignment supported by the PMD?
> Then, application has all the knowledge you want to expose before the configuration.

This may not work because there may be other restrictions besides alignment, which are
related to the hardware design. Therefore, it is difficult to describe all constraints
in a single field. Moreover, this approach seems to constrain the PMDs and HW to some
extent.
> 
>>>
>>>>>
>>>>>>> Maybe document that 0 means - no limitation by HW?
>>>>>>
>>>>>> Yes, there is no need to fill this filed for HW that has no restrictions on
>> it.
>>>>>> I'll add it in v4.
>>>>>>
>>>>>>> Must application read it in order to know if its datapath should
>>>>>>> handle
>>>>>> multi-segment buffers?
>>>>>>
>>>>>> I think it's more appropriate to use scattered_rx to determine if
>>>>>> multi- segment buffers should be handled.
>>>>>>
>>>>>>>
>>>>>>> Maybe it will be good to force application to configure scatter
>>>>>>> when this
>>>>>> field is valid and smaller than max_rx_pkt_len\max_lro.. (<= room
>> size)...
>>>>>
>>>>> Can you explain more what is the issue you came to solve?
>>>>
>>>> This HW information may be useful when there is some problems running
>>>> a application. This structure and related APIs can be used to expose
>>>> it at run time.
>>>>>
>>> OK
>>>
>>> .
>>>
> 
> 
> .
> 

Re: [dpdk-dev] [PATCH v3 1/4] ethdev: add a field for rxq info structure

[Matan Azrad]

Hi Chengchang

From: Chengchang Tang:
> Hi Matan
> 
> On 2020/9/6 21:45, Matan Azrad wrote:
> >
> > Hi  Chengchang
> >
> > From: Chengchang Tang:
> >> Hi, Matan
> >>
> >> On 2020/9/2 18:30, Matan Azrad wrote:
> >>> Hi Chengchang
> >>>
> >>> From: Chengchang Tang
> >>>> Hi, Matan
> >>>>
> >>>> On 2020/9/2 15:19, Matan Azrad wrote:
> >>>>>
> >>>>> Hi Chengchang
> >>>>>
> >>>>> From: Chengchang Tang
> >>>>>> Hi, Matan
> >>>>>>
> >>>>>> On 2020/9/1 23:33, Matan Azrad wrote:
> >>>>>>>
> >>>>>>> Hi Chengchang
> >>>>>>>
> >>>>>>> Please see some question below.
> >>>>>>>
> >>>>>>> From: Chengchang Tang
> >>>>>>>> Add a field named rx_buf_size in rte_eth_rxq_info to indicate
> >>>>>>>> the buffer size used in receiving packets for HW.
> >>>>>>>>
> >>>>>>>> In this way, upper-layer users can get this information by
> >>>>>>>> calling rte_eth_rx_queue_info_get.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Chengchang Tang
> <tangchengchang@huawei.com>
> >>>>>>>> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> >>>>>>>> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> >>>>>>>> ---
> >>>>>>>>  lib/librte_ethdev/rte_ethdev.h | 2 ++
> >>>>>>>>  1 file changed, 2 insertions(+)
> >>>>>>>>
<snip>
> >>> So the user can configure X and the driver will use Y!=X?
> >>
> >> Yes, it depends on the HW. In the queue setup API, it just checks
> >> whether the input is greater than the required minimum value. But HW
> >> usually has requirements for alignment and so on.
> >> So when X does not meet these requirements, PMDs will calculate a new
> >> value Y that meets these requirements to configure the hardware (Y <=
> >> X, to ensure no memory overflow occurs).
> >>> Should the application validate its own configurations after setting
> >>> them
> >> successfully?
> >>
> >> It depends on their own needs. The application should not be forced
> >> to verify it to avoid affecting the ease of use of PMDs. For some
> >> applications, they don't care about this value.
> >
> > I understand,
> > It looks me like a bad ping-pong between app and PMD (for all the
> > fields in the struct), And we should avoid adding fields to this structure if
> we can.
> >
> > What's about adding field in rte_eth_dev_info to expose the rx buffer
> alignment supported by the PMD?
> > Then, application has all the knowledge you want to expose before the
> configuration.
> 
> This may not work because there may be other restrictions besides
> alignment, which are related to the hardware design. Therefore, it is difficult
> to describe all constraints in a single field.
> Moreover, this approach seems to
> constrain the PMDs and HW to some extent.

Ok, so maybe other ethdev capability API to get the Rx buffer size adjustment by the PMD?
Don't you think it is important information to the application in order to decide the mempool buffer size \ enabling scatter?

In any case, I think you should add documentation in the RX setup API that the HW buf size may be changed by the PMD.

<snip>

Re: [dpdk-dev] [PATCH v3 1/4] ethdev: add a field for rxq info structure

[Chengchang Tang]

Hi Matan

On 2020/9/7 16:28, Matan Azrad wrote:
> 
> Hi Chengchang
> 
> From: Chengchang Tang:
>> Hi Matan
>>
>> On 2020/9/6 21:45, Matan Azrad wrote:
>>>
>>> Hi  Chengchang
>>>
>>> From: Chengchang Tang:
>>>> Hi, Matan
>>>>
>>>> On 2020/9/2 18:30, Matan Azrad wrote:
>>>>> Hi Chengchang
>>>>>
>>>>> From: Chengchang Tang
>>>>>> Hi, Matan
>>>>>>
>>>>>> On 2020/9/2 15:19, Matan Azrad wrote:
>>>>>>>
>>>>>>> Hi Chengchang
>>>>>>>
>>>>>>> From: Chengchang Tang
>>>>>>>> Hi, Matan
>>>>>>>>
>>>>>>>> On 2020/9/1 23:33, Matan Azrad wrote:
>>>>>>>>>
>>>>>>>>> Hi Chengchang
>>>>>>>>>
>>>>>>>>> Please see some question below.
>>>>>>>>>
>>>>>>>>> From: Chengchang Tang
>>>>>>>>>> Add a field named rx_buf_size in rte_eth_rxq_info to indicate
>>>>>>>>>> the buffer size used in receiving packets for HW.
>>>>>>>>>>
>>>>>>>>>> In this way, upper-layer users can get this information by
>>>>>>>>>> calling rte_eth_rx_queue_info_get.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Chengchang Tang
>> <tangchengchang@huawei.com>
>>>>>>>>>> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>>>>>>>>>> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>>>>>>>>> ---
>>>>>>>>>>  lib/librte_ethdev/rte_ethdev.h | 2 ++
>>>>>>>>>>  1 file changed, 2 insertions(+)
>>>>>>>>>>
> <snip>
>>>>> So the user can configure X and the driver will use Y!=X?
>>>>
>>>> Yes, it depends on the HW. In the queue setup API, it just checks
>>>> whether the input is greater than the required minimum value. But HW
>>>> usually has requirements for alignment and so on.
>>>> So when X does not meet these requirements, PMDs will calculate a new
>>>> value Y that meets these requirements to configure the hardware (Y <=
>>>> X, to ensure no memory overflow occurs).
>>>>> Should the application validate its own configurations after setting
>>>>> them
>>>> successfully?
>>>>
>>>> It depends on their own needs. The application should not be forced
>>>> to verify it to avoid affecting the ease of use of PMDs. For some
>>>> applications, they don't care about this value.
>>>
>>> I understand,
>>> It looks me like a bad ping-pong between app and PMD (for all the
>>> fields in the struct), And we should avoid adding fields to this structure if
>> we can.
>>>
>>> What's about adding field in rte_eth_dev_info to expose the rx buffer
>> alignment supported by the PMD?
>>> Then, application has all the knowledge you want to expose before the
>> configuration.
>>
>> This may not work because there may be other restrictions besides
>> alignment, which are related to the hardware design. Therefore, it is difficult
>> to describe all constraints in a single field.
>> Moreover, this approach seems to
>> constrain the PMDs and HW to some extent.
> 
> Ok, so maybe other ethdev capability API to get the Rx buffer size adjustment by the PMD?
> Don't you think it is important information to the application in order to decide the mempool buffer size \ enabling scatter?

I guess what you mean is that it's more like a capability, so the application should query it through a capability API.
If i understand correctly, I agree with that. But I think it's okay to use this structure to export queue related information
at runtime. It focuses on querying the current queue configuration.

And there seems to be no suitable API for querying this capability. Maybe we need to introduce a new API to do this. But
I'm not sure if it's really necessary.

> 
> In any case, I think you should add documentation in the RX setup API that the HW buf size may be changed by the PMD.

There is not a defined rule of how to configure Rx buffer size. That is, there is no specific method to configure the Rx
buffer size for applications. However, most PMDs configure the Rx buffer size base on the data size of the mempool. So,
if the description is added to the setup API, the method for configuring the Rx buffer size is determined. I think this
issue should involve more people in the discussion, maybe we should send a separate patch.
> 
> <snip>
> 
> .
> 

Re: [dpdk-dev] [PATCH v3 1/4] ethdev: add a field for rxq info structure

[Matan Azrad]

Hi Chengchang
From: Chengchang Tang:
> Hi Matan
> 
> On 2020/9/7 16:28, Matan Azrad wrote:
> >
> > Hi Chengchang
> >
> > From: Chengchang Tang:
> >> Hi Matan
> >>
> >> On 2020/9/6 21:45, Matan Azrad wrote:
> >>>
> >>> Hi  Chengchang
> >>>
> >>> From: Chengchang Tang:
> >>>> Hi, Matan
> >>>>
> >>>> On 2020/9/2 18:30, Matan Azrad wrote:
> >>>>> Hi Chengchang
> >>>>>
> >>>>> From: Chengchang Tang
> >>>>>> Hi, Matan
> >>>>>>
> >>>>>> On 2020/9/2 15:19, Matan Azrad wrote:
> >>>>>>>
> >>>>>>> Hi Chengchang
> >>>>>>>
> >>>>>>> From: Chengchang Tang
> >>>>>>>> Hi, Matan
> >>>>>>>>
> >>>>>>>> On 2020/9/1 23:33, Matan Azrad wrote:
> >>>>>>>>>
> >>>>>>>>> Hi Chengchang
> >>>>>>>>>
> >>>>>>>>> Please see some question below.
> >>>>>>>>>
> >>>>>>>>> From: Chengchang Tang
> >>>>>>>>>> Add a field named rx_buf_size in rte_eth_rxq_info to indicate
> >>>>>>>>>> the buffer size used in receiving packets for HW.
> >>>>>>>>>>
> >>>>>>>>>> In this way, upper-layer users can get this information by
> >>>>>>>>>> calling rte_eth_rx_queue_info_get.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Chengchang Tang
> >> <tangchengchang@huawei.com>
> >>>>>>>>>> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> >>>>>>>>>> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> >>>>>>>>>> ---
> >>>>>>>>>>  lib/librte_ethdev/rte_ethdev.h | 2 ++
> >>>>>>>>>>  1 file changed, 2 insertions(+)
> >>>>>>>>>>
> > <snip>
> >>>>> So the user can configure X and the driver will use Y!=X?
> >>>>
> >>>> Yes, it depends on the HW. In the queue setup API, it just checks
> >>>> whether the input is greater than the required minimum value. But
> >>>> HW usually has requirements for alignment and so on.
> >>>> So when X does not meet these requirements, PMDs will calculate a
> >>>> new value Y that meets these requirements to configure the hardware
> >>>> (Y <= X, to ensure no memory overflow occurs).
> >>>>> Should the application validate its own configurations after
> >>>>> setting them
> >>>> successfully?
> >>>>
> >>>> It depends on their own needs. The application should not be forced
> >>>> to verify it to avoid affecting the ease of use of PMDs. For some
> >>>> applications, they don't care about this value.
> >>>
> >>> I understand,
> >>> It looks me like a bad ping-pong between app and PMD (for all the
> >>> fields in the struct), And we should avoid adding fields to this
> >>> structure if
> >> we can.
> >>>
> >>> What's about adding field in rte_eth_dev_info to expose the rx
> >>> buffer
> >> alignment supported by the PMD?
> >>> Then, application has all the knowledge you want to expose before
> >>> the
> >> configuration.
> >>
> >> This may not work because there may be other restrictions besides
> >> alignment, which are related to the hardware design. Therefore, it is
> >> difficult to describe all constraints in a single field.
> >> Moreover, this approach seems to
> >> constrain the PMDs and HW to some extent.
> >
> > Ok, so maybe other ethdev capability API to get the Rx buffer size
> adjustment by the PMD?
> > Don't you think it is important information to the application in order to
> decide the mempool buffer size \ enabling scatter?
> 
> I guess what you mean is that it's more like a capability, so the application
> should query it through a capability API.

Yes.

> If i understand correctly, I agree with that. But I think it's okay to use this
> structure to export queue related information at runtime. It focuses on
> querying the current queue configuration.

Why do we need it If the capability(suggested above) say it already? 

> And there seems to be no suitable API for querying this capability. Maybe we
> need to introduce a new API to do this. But I'm not sure if it's really
> necessary.
 
For example if the user use room of size X in the RX mempool and the PMD configures X/2 because of HW limitation:
Don't you think it will affect the user configuration of mempool and scatter mode?

> > In any case, I think you should add documentation in the RX setup API that
> the HW buf size may be changed by the PMD.
> 
> There is not a defined rule of how to configure Rx buffer size. That is, there is
> no specific method to configure the Rx buffer size for applications. However,
> most PMDs configure the Rx buffer size base on the data size of the
> mempool. So, if the description is added to the setup API, the method for
> configuring the Rx buffer size is determined. I think this issue should involve
> more people in the discussion, maybe we should send a separate patch.

The mempool parameter of RX say it exactly, no?


> > <snip>
> >
> > .
> >

Re: [dpdk-dev] [PATCH v3 1/4] ethdev: add a field for rxq info structure

[Ferruh Yigit]

On 9/2/2020 8:19 AM, Matan Azrad wrote:
> 
> Hi Chengchang
> 
> From: Chengchang Tang
>> Hi, Matan
>>
>> On 2020/9/1 23:33, Matan Azrad wrote:
>>>
>>> Hi Chengchang
>>>
>>> Please see some question below.
>>>
>>> From: Chengchang Tang
>>>> Add a field named rx_buf_size in rte_eth_rxq_info to indicate the
>>>> buffer size used in receiving packets for HW.
>>>>
>>>> In this way, upper-layer users can get this information by calling
>>>> rte_eth_rx_queue_info_get.
>>>>
>>>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>>>> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>>>> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>>> ---
>>>>  lib/librte_ethdev/rte_ethdev.h | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/lib/librte_ethdev/rte_ethdev.h
>>>> b/lib/librte_ethdev/rte_ethdev.h index 70295d7..9fed5cb 100644
>>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>>> @@ -1420,6 +1420,8 @@ struct rte_eth_rxq_info {
>>>>         struct rte_eth_rxconf conf; /**< queue config parameters. */
>>>>         uint8_t scattered_rx;       /**< scattered packets RX supported. */
>>>>         uint16_t nb_desc;           /**< configured number of RXDs. */
>>>> +       /**< buffer size used for hardware when receive packets. */
>>>> +       uint16_t rx_buf_size;
>>>
>>> Is it the maximum supported Rx buffer by the HW?
>>> If yes, maybe max_rx_buf_size is better name?
>>
>> No, it is the Rx buffer size currently used by HW.
> 
> Doesn't it defined by the user? Using Rx queue mem-pool mbuf room size?
> 
> And it may be different per Rx queue....

There is no explicit configuration for Rx buffer size, PMDs does as you said
above and set mbuf data size as Rx buffer size, but this is not a defined rule,
technically PMD is free to select any size smaller than mbuf data size as their
Rx buffer size.
And this new field is to feed this configured Rx buffer size information back to
application.

> 
>> IMHO, the structure rte_eth_rxq_info and associated query API are mainly
>> used to query HW configurations at runtime or after queue is
>> configured/setup. Therefore, the content of this structure should be the
>> current HW configuration.
> 
> It looks me more like capabilities...
> The one which define the current configuration is the user by the configuration APIs(after reading the capabilities).
> 
> I don't think we have here all the current configurations, so what is special in this one?
> 
> 
>>> Maybe document that 0 means - no limitation by HW?
>>
>> Yes, there is no need to fill this filed for HW that has no restrictions on it.
>> I'll add it in v4.
>>
>>> Must application read it in order to know if its datapath should handle
>> multi-segment buffers?
>>
>> I think it's more appropriate to use scattered_rx to determine if multi-
>> segment buffers should be handled.
>>
>>>
>>> Maybe it will be good to force application to configure scatter when this
>> field is valid and smaller than max_rx_pkt_len\max_lro.. (<= room size)...
> 
> Can you explain more what is the issue you came to solve?
> 
>>>
>>>>  } __rte_cache_min_aligned;
>>>>
>>>>  /**
>>
> 

Re: [dpdk-dev] [PATCH v3 1/4] ethdev: add a field for rxq info structure

[Ferruh Yigit]

On 9/2/2020 4:52 AM, Chengchang Tang wrote:
> Hi, Matan
> 
> On 2020/9/1 23:33, Matan Azrad wrote:
>>
>> Hi Chengchang
>>
>> Please see some question below.
>>
>> From: Chengchang Tang
>>> Add a field named rx_buf_size in rte_eth_rxq_info to indicate the buffer size
>>> used in receiving packets for HW.
>>>
>>> In this way, upper-layer users can get this information by calling
>>> rte_eth_rx_queue_info_get.
>>>
>>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>>> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>>> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>> ---
>>>  lib/librte_ethdev/rte_ethdev.h | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>>> index 70295d7..9fed5cb 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>> @@ -1420,6 +1420,8 @@ struct rte_eth_rxq_info {
>>>         struct rte_eth_rxconf conf; /**< queue config parameters. */
>>>         uint8_t scattered_rx;       /**< scattered packets RX supported. */
>>>         uint16_t nb_desc;           /**< configured number of RXDs. */
>>> +       /**< buffer size used for hardware when receive packets. */
>>> +       uint16_t rx_buf_size;
>>
>> Is it the maximum supported Rx buffer by the HW?
>> If yes, maybe max_rx_buf_size is better name?
> 
> No, it is the Rx buffer size currently used by HW.
> 
> IMHO, the structure rte_eth_rxq_info and associated query API are mainly used to query HW configurations
> at runtime or after queue is configured/setup. Therefore, the content of this structure should be the
> current HW configuration.
> 
>> Maybe document that 0 means - no limitation by HW?
> 
> Yes, there is no need to fill this filed for HW that has no restrictions on it.
> I'll add it in v4.

This field in not for max Rx buffer size, so HW restriction doesn't really apply
here.
But as far as I remember it has been decided that the field will be optional for
PMDs, agree to document that part.

> 
>> Must application read it in order to know if its datapath should handle multi-segment buffers?
> 
> I think it's more appropriate to use scattered_rx to determine if multi-segment buffers should be handled.
> 
>>
>> Maybe it will be good to force application to configure scatter when this field is valid and smaller than max_rx_pkt_len\max_lro.. (<= room size)...
>>
>>
>>>  } __rte_cache_min_aligned;
>>>
>>>  /**
> 
> 

Re: [dpdk-dev] [PATCH v3 1/4] ethdev: add a field for rxq info structure

[Ferruh Yigit]

On 8/29/2020 8:13 AM, Chengchang Tang wrote:
> Add a field named rx_buf_size in rte_eth_rxq_info to indicate the buffer
> size used in receiving packets for HW.
> 
> In this way, upper-layer users can get this information by calling
> rte_eth_rx_queue_info_get.
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
>  lib/librte_ethdev/rte_ethdev.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 70295d7..9fed5cb 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1420,6 +1420,8 @@ struct rte_eth_rxq_info {
>  	struct rte_eth_rxconf conf; /**< queue config parameters. */
>  	uint8_t scattered_rx;       /**< scattered packets RX supported. */
>  	uint16_t nb_desc;           /**< configured number of RXDs. */
> +	/**< buffer size used for hardware when receive packets. */
> +	uint16_t rx_buf_size;
>  } __rte_cache_min_aligned;
> 
>  /**
> --
> 2.7.4
> 

Can you please removed the relevant deprecation notice in this patch?

Re: [dpdk-dev] [PATCH v3 1/4] ethdev: add a field for rxq info structure

[Chengchang Tang]

On 2020/9/3 23:01, Ferruh Yigit wrote:
> On 8/29/2020 8:13 AM, Chengchang Tang wrote:
>> Add a field named rx_buf_size in rte_eth_rxq_info to indicate the buffer
>> size used in receiving packets for HW.
>>
>> In this way, upper-layer users can get this information by calling
>> rte_eth_rx_queue_info_get.
>>
>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
>>  lib/librte_ethdev/rte_ethdev.h | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>> index 70295d7..9fed5cb 100644
>> --- a/lib/librte_ethdev/rte_ethdev.h
>> +++ b/lib/librte_ethdev/rte_ethdev.h
>> @@ -1420,6 +1420,8 @@ struct rte_eth_rxq_info {
>>  	struct rte_eth_rxconf conf; /**< queue config parameters. */
>>  	uint8_t scattered_rx;       /**< scattered packets RX supported. */
>>  	uint16_t nb_desc;           /**< configured number of RXDs. */
>> +	/**< buffer size used for hardware when receive packets. */
>> +	uint16_t rx_buf_size;
>>  } __rte_cache_min_aligned;
>>
>>  /**
>> --
>> 2.7.4
>>
> 
> Can you please removed the relevant deprecation notice in this patch?

OK, I will remove it in v4
> .
> 

Re: [dpdk-dev] [PATCH v3 1/4] ethdev: add a field for rxq info structure

[Bruce Richardson]

On Sat, Aug 29, 2020 at 03:13:16PM +0800, Chengchang Tang wrote:
> Add a field named rx_buf_size in rte_eth_rxq_info to indicate the buffer
> size used in receiving packets for HW.
> 
> In this way, upper-layer users can get this information by calling
> rte_eth_rx_queue_info_get.
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
>  lib/librte_ethdev/rte_ethdev.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 70295d7..9fed5cb 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1420,6 +1420,8 @@ struct rte_eth_rxq_info {
>  	struct rte_eth_rxconf conf; /**< queue config parameters. */
>  	uint8_t scattered_rx;       /**< scattered packets RX supported. */
>  	uint16_t nb_desc;           /**< configured number of RXDs. */
> +	/**< buffer size used for hardware when receive packets. */
> +	uint16_t rx_buf_size;
>  } __rte_cache_min_aligned;
> 
Since this is breaking the ABI, this looks like the perfect opportunity to
add in a qinfo_size parameter to rte_eth_rx_queue_info_get() call which
allows ABI sanity-checking. Also, if passed through to the individual
drivers, allows them to make ABI determinations since driver functions
cannot be versioned, i.e. the driver info function cannot know whether it
has been called by queue_info_v21 or queue_info_v22.

/Bruce

Re: [dpdk-dev] [PATCH v3 1/4] ethdev: add a field for rxq info structure

[Ferruh Yigit]

On 9/3/2020 4:35 PM, Bruce Richardson wrote:
> On Sat, Aug 29, 2020 at 03:13:16PM +0800, Chengchang Tang wrote:
>> Add a field named rx_buf_size in rte_eth_rxq_info to indicate the buffer
>> size used in receiving packets for HW.
>>
>> In this way, upper-layer users can get this information by calling
>> rte_eth_rx_queue_info_get.
>>
>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
>>  lib/librte_ethdev/rte_ethdev.h | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>> index 70295d7..9fed5cb 100644
>> --- a/lib/librte_ethdev/rte_ethdev.h
>> +++ b/lib/librte_ethdev/rte_ethdev.h
>> @@ -1420,6 +1420,8 @@ struct rte_eth_rxq_info {
>>  	struct rte_eth_rxconf conf; /**< queue config parameters. */
>>  	uint8_t scattered_rx;       /**< scattered packets RX supported. */
>>  	uint16_t nb_desc;           /**< configured number of RXDs. */
>> +	/**< buffer size used for hardware when receive packets. */
>> +	uint16_t rx_buf_size;
>>  } __rte_cache_min_aligned;
>>
> Since this is breaking the ABI, this looks like the perfect opportunity to
> add in a qinfo_size parameter to rte_eth_rx_queue_info_get() call which
> allows ABI sanity-checking. Also, if passed through to the individual
> drivers, allows them to make ABI determinations since driver functions
> cannot be versioned, i.e. the driver info function cannot know whether it
> has been called by queue_info_v21 or queue_info_v22.
> 

Current approach we have is to detect the ABI breakage before release and either
block it or use ABI versioning.
What you suggest to add runtime checks assumes application can call old and new
APIs, which is not our case and not sure what this runtime check adds.

The option to use runtime check between library and driver can be needed if
library and driver from different DPDK versions can be used together, I think
enabling this can bring more complexity and better to use all components of a
DPDK release together.

And I think this kind of runtime checks should be discussed for all or none
APIs, instead of having them only for some APIs.

Re: [dpdk-dev] [PATCH v3 1/4] ethdev: add a field for rxq info structure

[Bruce Richardson]

On Fri, Sep 04, 2020 at 03:25:26PM +0100, Ferruh Yigit wrote:
> On 9/3/2020 4:35 PM, Bruce Richardson wrote:
> > On Sat, Aug 29, 2020 at 03:13:16PM +0800, Chengchang Tang wrote:
> >> Add a field named rx_buf_size in rte_eth_rxq_info to indicate the buffer
> >> size used in receiving packets for HW.
> >>
> >> In this way, upper-layer users can get this information by calling
> >> rte_eth_rx_queue_info_get.
> >>
> >> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> >> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> >> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> >> ---
> >>  lib/librte_ethdev/rte_ethdev.h | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> >> index 70295d7..9fed5cb 100644
> >> --- a/lib/librte_ethdev/rte_ethdev.h
> >> +++ b/lib/librte_ethdev/rte_ethdev.h
> >> @@ -1420,6 +1420,8 @@ struct rte_eth_rxq_info {
> >>  	struct rte_eth_rxconf conf; /**< queue config parameters. */
> >>  	uint8_t scattered_rx;       /**< scattered packets RX supported. */
> >>  	uint16_t nb_desc;           /**< configured number of RXDs. */
> >> +	/**< buffer size used for hardware when receive packets. */
> >> +	uint16_t rx_buf_size;
> >>  } __rte_cache_min_aligned;
> >>
> > Since this is breaking the ABI, this looks like the perfect opportunity to
> > add in a qinfo_size parameter to rte_eth_rx_queue_info_get() call which
> > allows ABI sanity-checking. Also, if passed through to the individual
> > drivers, allows them to make ABI determinations since driver functions
> > cannot be versioned, i.e. the driver info function cannot know whether it
> > has been called by queue_info_v21 or queue_info_v22.
> > 
> 
> Current approach we have is to detect the ABI breakage before release and either
> block it or use ABI versioning.
> What you suggest to add runtime checks assumes application can call old and new
> APIs, which is not our case and not sure what this runtime check adds.
> 
> The option to use runtime check between library and driver can be needed if
> library and driver from different DPDK versions can be used together, I think
> enabling this can bring more complexity and better to use all components of a
> DPDK release together.
> 
> And I think this kind of runtime checks should be discussed for all or none
> APIs, instead of having them only for some APIs.

No the main concern here is to avoid the kind of ABI issues that hit the
crypto tree in a recent release. Passing the length of the array around
would actually make the regular versioning simpler (as I think we wouldn't
need to use the fancy linker versioning), but the main benefit is if that
info can be passed through to the driver layer, since driver functions
called via ethdev cannot be versioned.

/Bruce

Re: [dpdk-dev] [PATCH v3 1/4] ethdev: add a field for rxq info structure

[Bruce Richardson]

On Fri, Sep 04, 2020 at 03:25:26PM +0100, Ferruh Yigit wrote:
> On 9/3/2020 4:35 PM, Bruce Richardson wrote:
> > On Sat, Aug 29, 2020 at 03:13:16PM +0800, Chengchang Tang wrote:
> >> Add a field named rx_buf_size in rte_eth_rxq_info to indicate the buffer
> >> size used in receiving packets for HW.
> >>
> >> In this way, upper-layer users can get this information by calling
> >> rte_eth_rx_queue_info_get.
> >>
> >> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> >> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> >> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> >> ---
> >>  lib/librte_ethdev/rte_ethdev.h | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> >> index 70295d7..9fed5cb 100644
> >> --- a/lib/librte_ethdev/rte_ethdev.h
> >> +++ b/lib/librte_ethdev/rte_ethdev.h
> >> @@ -1420,6 +1420,8 @@ struct rte_eth_rxq_info {
> >>  	struct rte_eth_rxconf conf; /**< queue config parameters. */
> >>  	uint8_t scattered_rx;       /**< scattered packets RX supported. */
> >>  	uint16_t nb_desc;           /**< configured number of RXDs. */
> >> +	/**< buffer size used for hardware when receive packets. */
> >> +	uint16_t rx_buf_size;
> >>  } __rte_cache_min_aligned;
> >>
> > Since this is breaking the ABI, this looks like the perfect opportunity to
> > add in a qinfo_size parameter to rte_eth_rx_queue_info_get() call which
> > allows ABI sanity-checking. Also, if passed through to the individual
> > drivers, allows them to make ABI determinations since driver functions
> > cannot be versioned, i.e. the driver info function cannot know whether it
> > has been called by queue_info_v21 or queue_info_v22.
> > 
> 
> Current approach we have is to detect the ABI breakage before release and either
> block it or use ABI versioning.
> What you suggest to add runtime checks assumes application can call old and new
> APIs, which is not our case and not sure what this runtime check adds.
> 
> The option to use runtime check between library and driver can be needed if
> library and driver from different DPDK versions can be used together, I think
> enabling this can bring more complexity and better to use all components of a
> DPDK release together.

Allow me to explain a bit further. For example, we add a new field to the
end of the configuration struct or the info struct, which changes the size
of the structure. We use function versioning to allow old and new ethdev
config APIs. The new API function just works passing the structure through
to the driver API. However, the old API implementation has a problem,
because it has just been passed a structure that is too small for use by
the driver functions, so it can't just pass it along. Therefore, the code
has two choices (that I am aware of):

1. allocate a new structure of the correct size, and call the driver API
with that. Once it returns, copy the fields over to the old structure to
return to the user.
2. create a new function pointer in the ops array for legacy compatibility
and have each driver implement a function to work with the old structure.

The alternative scheme is just to pass around the structure size. The
configure ethdev API takes the pointer and size, and does not need changing
if it does not use any new fields, just passes them through to the driver
configure API. Similarly there, any drivers that don't access the new
fields don't need changing, while those that do access them just need to
check the struct size before doing so to see what version of the structure
has been received.

> 
> And I think this kind of runtime checks should be discussed for all or none
> APIs, instead of having them only for some APIs.

Yes, I agree with that.
Incidentally, this same idea was covered in the context of system calls at
the linux plumbers conf:
https://linuxplumbersconf.org/event/7/contributions/657/attachments/639/1159/extensible_syscalls.pdf

Regards,
/Bruce

[dpdk-dev] [PATCH v3 2/4] app/testpmd: add Rx buffer size display in queue info query

[Chengchang Tang]

Add Rx buffer size to queue info querry cmd so that the user can get the
buffer length used by HW queue for receiving packets.

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
---
 app/test-pmd/config.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 30bee33..b432ac6 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -452,6 +452,7 @@ rx_queue_infos_display(portid_t port_id, uint16_t queue_id)
 		(qinfo.conf.rx_deferred_start != 0) ? "on" : "off");
 	printf("\nRX scattered packets: %s",
 		(qinfo.scattered_rx != 0) ? "on" : "off");
+	printf("\nRX buffer size: %hu", qinfo.rx_buf_size);
 	printf("\nNumber of RXDs: %hu", qinfo.nb_desc);

 	if (rte_eth_rx_burst_mode_get(port_id, queue_id, &mode) == 0)
--
2.7.4

[dpdk-dev] [PATCH v3 3/4] app/procinfo: add Rx buffer size to --show-port

[Chengchang Tang]

Add Rx buffer size to show_port function to display it in the port PMD
information so that the user can get the buffer length used by HW queue
of receiving packets.

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
---
 app/proc-info/main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/app/proc-info/main.c b/app/proc-info/main.c
index abeca4a..e840dea 100644
--- a/app/proc-info/main.c
+++ b/app/proc-info/main.c
@@ -711,11 +711,13 @@ show_port(void)
 			if (ret == 0) {
 				printf("\t  -- queue %d rx scatter %d"
 						" descriptors %d"
+						" rx buffer size %d"
 						" offloads 0x%"PRIx64
 						" mempool socket %d\n",
 						j,
 						queue_info.scattered_rx,
 						queue_info.nb_desc,
+						queue_info.rx_buf_size,
 						queue_info.conf.offloads,
 						queue_info.mp->socket_id);
 			}
--
2.7.4

[dpdk-dev] [PATCH v3 4/4] net/hns3: add Rx buffer size to Rx qinfo query

[Chengchang Tang]

Report hns3 PMD configured Rx buffer size in Rx queue information query.

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
---
 drivers/net/hns3/hns3_rxtx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
index fc1a256..d67c5ad 100644
--- a/drivers/net/hns3/hns3_rxtx.c
+++ b/drivers/net/hns3/hns3_rxtx.c
@@ -2830,6 +2830,8 @@ hns3_rxq_info_get(struct rte_eth_dev *dev, uint16_t queue_id,
 	qinfo->mp = rxq->mb_pool;
 	qinfo->nb_desc = rxq->nb_rx_desc;
 	qinfo->scattered_rx = dev->data->scattered_rx;
+	/* Report the HW Rx buffer length to user */
+	qinfo->rx_buf_size = rxq->rx_buf_len;

 	/*
 	 * If there are no available Rx buffer descriptors, incoming packets
--
2.7.4

[dpdk-dev] [PATCH v4 0/5] add Rx buffer size for rxq info structure

[Chengchang Tang]

In common practice, PMD configure the Rx buffer size which indicate the
buffer length could be used for HW in receiving packets according to the
data room size of the object in mempool. But in fact, the final value is
related to the specifications of HW, and its values will affect the number
of fragments in receiving packets when scatter is enabled. By the way,
some PMDs may force to enable scatter when the MTU is bigger than the HW
Rx buffer size.

At present, we have no way to expose relevant information to upper layer
users. So, add a field named rx_buf_size in rte_eth_rxq_info to indicate
the buffer size used in receiving packets for HW. And this field is
optional, so there is no need to forcibly update all PMDs.

This patchset also update testpmd, proc-info tools and add hns3 PMD
implementation.

v4:
  - remove deprecation notice once changes applied

Chengchang Tang (5):
  ethdev: add a field for rxq info structure
  app/testpmd: add Rx buffer size display in queue info query
  app/procinfo: add Rx buffer size to --show-port
  net/hns3: add Rx buffer size to Rx qinfo query
  doc: remove rxq info structure deprecation notice

 app/proc-info/main.c                 | 2 ++
 app/test-pmd/config.c                | 1 +
 doc/guides/rel_notes/deprecation.rst | 5 -----
 drivers/net/hns3/hns3_rxtx.c         | 2 ++
 lib/librte_ethdev/rte_ethdev.h       | 2 ++
 5 files changed, 7 insertions(+), 5 deletions(-)

--
2.7.4

[dpdk-dev] [PATCH v4 1/5] ethdev: add a field for rxq info structure

[Chengchang Tang]

Add a field named rx_buf_size in rte_eth_rxq_info to indicate the buffer
size used in receiving packets for HW.

In this way, upper-layer users can get this information by calling
rte_eth_rx_queue_info_get.

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 lib/librte_ethdev/rte_ethdev.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 70295d7..859111a 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1420,6 +1420,8 @@ struct rte_eth_rxq_info {
 	struct rte_eth_rxconf conf; /**< queue config parameters. */
 	uint8_t scattered_rx;       /**< scattered packets RX supported. */
 	uint16_t nb_desc;           /**< configured number of RXDs. */
+	/**< Buffer size used for HW when receive packets. */
+	uint16_t rx_buf_size;
 } __rte_cache_min_aligned;

 /**
--
2.7.4

Re: [dpdk-dev] [PATCH v4 1/5] ethdev: add a field for rxq info structure

[Stephen Hemminger]

On Sat, 5 Sep 2020 17:07:30 +0800
Chengchang Tang <tangchengchang@huawei.com> wrote:

> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 70295d7..859111a 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1420,6 +1420,8 @@ struct rte_eth_rxq_info {
>  	struct rte_eth_rxconf conf; /**< queue config parameters. */
>  	uint8_t scattered_rx;       /**< scattered packets RX supported. */
>  	uint16_t nb_desc;           /**< configured number of RXDs. */
> +	/**< Buffer size used for HW when receive packets. */
> +	uint16_t rx_buf_size;

Minor suggestion.
If you shorten the comment (which is not that useful) and use the same
case, then it will fit on the same line and look like the rest of the code.

	uint16_t rx_buf_size;      /**< hardware receive buffer size */

[dpdk-dev] [PATCH v4 2/5] app/testpmd: add Rx buffer size display in queue info query

[Chengchang Tang]

Add Rx buffer size to queue info querry cmd so that the user can get the
buffer length used by HW queue for receiving packets.

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
---
 app/test-pmd/config.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 30bee33..b432ac6 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -452,6 +452,7 @@ rx_queue_infos_display(portid_t port_id, uint16_t queue_id)
 		(qinfo.conf.rx_deferred_start != 0) ? "on" : "off");
 	printf("\nRX scattered packets: %s",
 		(qinfo.scattered_rx != 0) ? "on" : "off");
+	printf("\nRX buffer size: %hu", qinfo.rx_buf_size);
 	printf("\nNumber of RXDs: %hu", qinfo.nb_desc);

 	if (rte_eth_rx_burst_mode_get(port_id, queue_id, &mode) == 0)
--
2.7.4

Re: [dpdk-dev] [PATCH v4 2/5] app/testpmd: add Rx buffer size display in queue info query

[Ferruh Yigit]

On 9/5/2020 10:07 AM, Chengchang Tang wrote:
> Add Rx buffer size to queue info querry cmd so that the user can get the
> buffer length used by HW queue for receiving packets.
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> ---
>   app/test-pmd/config.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 30bee33..b432ac6 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -452,6 +452,7 @@ rx_queue_infos_display(portid_t port_id, uint16_t queue_id)
>   		(qinfo.conf.rx_deferred_start != 0) ? "on" : "off");
>   	printf("\nRX scattered packets: %s",
>   		(qinfo.scattered_rx != 0) ? "on" : "off");
> +	printf("\nRX buffer size: %hu", qinfo.rx_buf_size);

Since this field is optional for PMD to fill, it may be confusing to 
display buffer size as "0".
What do you think print this value when "qinfo.rx_buf_size != 0"?

Re: [dpdk-dev] [PATCH v4 2/5] app/testpmd: add Rx buffer size display in queue info query

[Chengchang Tang]

On 2020/9/18 16:54, Ferruh Yigit wrote:
> On 9/5/2020 10:07 AM, Chengchang Tang wrote:
>> Add Rx buffer size to queue info querry cmd so that the user can get the
>> buffer length used by HW queue for receiving packets.
>>
>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>> ---
>>   app/test-pmd/config.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>> index 30bee33..b432ac6 100644
>> --- a/app/test-pmd/config.c
>> +++ b/app/test-pmd/config.c
>> @@ -452,6 +452,7 @@ rx_queue_infos_display(portid_t port_id, uint16_t queue_id)
>>           (qinfo.conf.rx_deferred_start != 0) ? "on" : "off");
>>       printf("\nRX scattered packets: %s",
>>           (qinfo.scattered_rx != 0) ? "on" : "off");
>> +    printf("\nRX buffer size: %hu", qinfo.rx_buf_size);
> 
> Since this field is optional for PMD to fill, it may be confusing to display buffer size as "0".
> What do you think print this value when "qinfo.rx_buf_size != 0"?

Agree, it will be modified in the next version.
> 
> .
> 

[dpdk-dev] [PATCH v4 3/5] app/procinfo: add Rx buffer size to --show-port

[Chengchang Tang]

Add Rx buffer size to show_port function to display it in the port PMD
information so that the user can get the buffer length used by HW queue
of receiving packets.

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
---
 app/proc-info/main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/app/proc-info/main.c b/app/proc-info/main.c
index abeca4a..e840dea 100644
--- a/app/proc-info/main.c
+++ b/app/proc-info/main.c
@@ -711,11 +711,13 @@ show_port(void)
 			if (ret == 0) {
 				printf("\t  -- queue %d rx scatter %d"
 						" descriptors %d"
+						" rx buffer size %d"
 						" offloads 0x%"PRIx64
 						" mempool socket %d\n",
 						j,
 						queue_info.scattered_rx,
 						queue_info.nb_desc,
+						queue_info.rx_buf_size,
 						queue_info.conf.offloads,
 						queue_info.mp->socket_id);
 			}
--
2.7.4

Re: [dpdk-dev] [PATCH v4 3/5] app/procinfo: add Rx buffer size to --show-port

[Stephen Hemminger]

On Sat, 5 Sep 2020 17:07:32 +0800
Chengchang Tang <tangchengchang@huawei.com> wrote:

>  				printf("\t  -- queue %d rx scatter %d"
>  						" descriptors %d"
> +						" rx buffer size %d"
>  						" offloads 0x%"PRIx64
>  						" mempool socket %d\n",
>  						j,
>  						queue_info.scattered_rx,
>  						queue_info.nb_desc,
> +						queue_info.rx_buf_size,
>  						queue_info.conf.offloads,
>  						queue_info.mp->socket_id);
>  			}

These should be using %u and need space after " for PRIx64
Why not:
  				printf("\t  -- queue %u rx scatter %u"
  						" descriptors %u"
 						" rx buffer size %u"
  						" offloads %#" PRIx64
  						" mempool socket %d\n",

Re: [dpdk-dev] [PATCH v4 3/5] app/procinfo: add Rx buffer size to --show-port

[Chengchang Tang]

On 2020/9/6 0:59, Stephen Hemminger wrote:
> On Sat, 5 Sep 2020 17:07:32 +0800
> Chengchang Tang <tangchengchang@huawei.com> wrote:
> 
>>  				printf("\t  -- queue %d rx scatter %d"
>>  						" descriptors %d"
>> +						" rx buffer size %d"
>>  						" offloads 0x%"PRIx64
>>  						" mempool socket %d\n",
>>  						j,
>>  						queue_info.scattered_rx,
>>  						queue_info.nb_desc,
>> +						queue_info.rx_buf_size,
>>  						queue_info.conf.offloads,
>>  						queue_info.mp->socket_id);
>>  			}
> 
> These should be using %u and need space after " for PRIx64
> Why not:
>   				printf("\t  -- queue %u rx scatter %u"
>   						" descriptors %u"
>  						" rx buffer size %u"
>   						" offloads %#" PRIx64
>   						" mempool socket %d\n",
> 

OK, I will fix these in next version.

Thanks

Re: [dpdk-dev] [PATCH v4 3/5] app/procinfo: add Rx buffer size to --show-port

[Stephen Hemminger]

On Mon, 7 Sep 2020 17:14:48 +0800
Chengchang Tang <tangchengchang@huawei.com> wrote:

> On 2020/9/6 0:59, Stephen Hemminger wrote:
> > On Sat, 5 Sep 2020 17:07:32 +0800
> > Chengchang Tang <tangchengchang@huawei.com> wrote:
> >   
> >>  				printf("\t  -- queue %d rx scatter %d"
> >>  						" descriptors %d"
> >> +						" rx buffer size %d"
> >>  						" offloads 0x%"PRIx64
> >>  						" mempool socket %d\n",
> >>  						j,
> >>  						queue_info.scattered_rx,
> >>  						queue_info.nb_desc,
> >> +						queue_info.rx_buf_size,
> >>  						queue_info.conf.offloads,
> >>  						queue_info.mp->socket_id);
> >>  			}  
> > 
> > These should be using %u and need space after " for PRIx64
> > Why not:
> >   				printf("\t  -- queue %u rx scatter %u"
> >   						" descriptors %u"
> >  						" rx buffer size %u"
> >   						" offloads %#" PRIx64
> >   						" mempool socket %d\n",
> >   
> 
> OK, I will fix these in next version.
> 
> Thanks

NAK, these is superseded by.

Please look at the new improved procinfo still stuck in patchwork
https://patchwork.dpdk.org/patch/74960/

Let's put the buffer size there.

Re: [dpdk-dev] [PATCH v4 3/5] app/procinfo: add Rx buffer size to --show-port

[Chengchang Tang]

Hi, All
These Patches improved procinfo was stuck in patchwork:
https://patchwork.dpdk.org/patch/74960/

These patches look good for me. Will these be applied?

I'm not sure what to do next. If these will be applied, I need modified
the code.

On 2020/9/19 6:11, Stephen Hemminger wrote:
> On Mon, 7 Sep 2020 17:14:48 +0800
> Chengchang Tang <tangchengchang@huawei.com> wrote:
> 
>> On 2020/9/6 0:59, Stephen Hemminger wrote:
>>> On Sat, 5 Sep 2020 17:07:32 +0800
>>> Chengchang Tang <tangchengchang@huawei.com> wrote:
>>>   
>>>>  				printf("\t  -- queue %d rx scatter %d"
>>>>  						" descriptors %d"
>>>> +						" rx buffer size %d"
>>>>  						" offloads 0x%"PRIx64
>>>>  						" mempool socket %d\n",
>>>>  						j,
>>>>  						queue_info.scattered_rx,
>>>>  						queue_info.nb_desc,
>>>> +						queue_info.rx_buf_size,
>>>>  						queue_info.conf.offloads,
>>>>  						queue_info.mp->socket_id);
>>>>  			}  
>>>
>>> These should be using %u and need space after " for PRIx64
>>> Why not:
>>>   				printf("\t  -- queue %u rx scatter %u"
>>>   						" descriptors %u"
>>>  						" rx buffer size %u"
>>>   						" offloads %#" PRIx64
>>>   						" mempool socket %d\n",
>>>   
>>
>> OK, I will fix these in next version.
>>
>> Thanks
> 
> NAK, these is superseded by.
> 
> Please look at the new improved procinfo still stuck in patchwork
> https://patchwork.dpdk.org/patch/74960/
> 
> Let's put the buffer size there.
> 
> .
> 

Re: [dpdk-dev] [PATCH v4 3/5] app/procinfo: add Rx buffer size to --show-port

[Ferruh Yigit]

On 9/21/2020 3:06 AM, Chengchang Tang wrote:
> Hi, All
> These Patches improved procinfo was stuck in patchwork:
> https://patchwork.dpdk.org/patch/74960/
> 
> These patches look good for me. Will these be applied?
> 
> I'm not sure what to do next. If these will be applied, I need modified
> the code.

Hi Chengchang,

I suggest don't hold your patches and send your next version on top of 
latest head of git repo, based on which patch applied first we can 
resolve the conflict accordingly.

[dpdk-dev] [PATCH v4 4/5] net/hns3: add Rx buffer size to Rx qinfo query

[Chengchang Tang]

Report hns3 PMD configured Rx buffer size in Rx queue information query.

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
---
 drivers/net/hns3/hns3_rxtx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
index fc1a256..d67c5ad 100644
--- a/drivers/net/hns3/hns3_rxtx.c
+++ b/drivers/net/hns3/hns3_rxtx.c
@@ -2830,6 +2830,8 @@ hns3_rxq_info_get(struct rte_eth_dev *dev, uint16_t queue_id,
 	qinfo->mp = rxq->mb_pool;
 	qinfo->nb_desc = rxq->nb_rx_desc;
 	qinfo->scattered_rx = dev->data->scattered_rx;
+	/* Report the HW Rx buffer length to user */
+	qinfo->rx_buf_size = rxq->rx_buf_len;

 	/*
 	 * If there are no available Rx buffer descriptors, incoming packets
--
2.7.4

[dpdk-dev] [PATCH v4 5/5] doc: remove rxq info structure deprecation notice

[Chengchang Tang]

The change has been applied, so remove the notice.

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
---
 doc/guides/rel_notes/deprecation.rst | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 345c38d..b6d57b9 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -215,11 +215,6 @@ Deprecation Notices
   specified lengths into the buffers allocated from the specified
   memory pools. The backward compatibility to existing API is preserved.

-* ethdev: The ``struct rte_eth_rxq_info`` will be modified to include
-  a new optional field, indicating the buffer size used in receiving packets
-  for HW. This change is planned for 20.11. For more details:
-  https://mails.dpdk.org/archives/dev/2020-July/176135.html.
-
 * ethdev: ``rx_descriptor_done`` dev_ops and ``rte_eth_rx_descriptor_done``
   will be deprecated in 20.11 and will be removed in 21.11.
   Existing ``rte_eth_rx_descriptor_status`` and ``rte_eth_tx_descriptor_status``
--
2.7.4

Re: [dpdk-dev] [PATCH v4 5/5] doc: remove rxq info structure deprecation notice

[Thomas Monjalon]

05/09/2020 11:07, Chengchang Tang:
> The change has been applied, so remove the notice.

It should be atomic with the patch doing the change,
i.e. you can squash.

I think the oneline patches in testpmd and procinfo
can probably be squashed with the ethdev patch as well.

Thanks

Re: [dpdk-dev] [PATCH v4 5/5] doc: remove rxq info structure deprecation notice

[Chengchang Tang]

On 2020/9/6 0:33, Thomas Monjalon wrote:
> 05/09/2020 11:07, Chengchang Tang:
>> The change has been applied, so remove the notice.
> 
> It should be atomic with the patch doing the change,
> i.e. you can squash.
> 
> I think the oneline patches in testpmd and procinfo
> can probably be squashed with the ethdev patch as well.

OK, I will squash them in next version.

Thanks

[dpdk-dev] [PATCH v5 0/2] add Rx buffer size for rxq info structure

[Chengchang Tang]

In common practice, PMD configure the Rx buffer size which indicate the
buffer length could be used for HW in receiving packets according to the
data room size of the object in mempool. But in fact, the final value is
related to the specifications of HW, and its values will affect the number
of fragments in receiving packets when scatter is enabled. By the way,
some PMDs may force to enable scatter when the MTU is bigger than the HW
Rx buffer size.

At present, we have no way to expose relevant information to upper layer
users. So, add a field named rx_buf_size in rte_eth_rxq_info to indicate
the buffer size used in receiving packets for HW. And this field is
optional, so there is no need to forcibly update all PMDs.

This patchset also update testpmd, proc-info tools and add hns3 PMD
implementation.

v5:
  - squash oneline app patches and deprecation remove with ethdev patch

Chengchang Tang (2):
  ethdev: add a field for rxq info structure
  net/hns3: add Rx buffer size to Rx qinfo query

 app/proc-info/main.c                 | 13 +++++++++----
 app/test-pmd/config.c                |  2 ++
 doc/guides/rel_notes/deprecation.rst |  5 -----
 drivers/net/hns3/hns3_rxtx.c         |  2 ++
 lib/librte_ethdev/rte_ethdev.h       |  1 +
 5 files changed, 14 insertions(+), 9 deletions(-)

--
2.7.4

[dpdk-dev] [PATCH v5 1/2] ethdev: add a field for rxq info structure

[Chengchang Tang]

Add a field named rx_buf_size in rte_eth_rxq_info to indicate the buffer
size used in receiving packets for HW.

In this way, upper-layer users can get this information by calling
rte_eth_rx_queue_info_get.

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 app/proc-info/main.c                 | 13 +++++++++----
 app/test-pmd/config.c                |  2 ++
 doc/guides/rel_notes/deprecation.rst |  5 -----
 lib/librte_ethdev/rte_ethdev.h       |  1 +
 4 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/app/proc-info/main.c b/app/proc-info/main.c
index 0b030d3..64fb83b 100644
--- a/app/proc-info/main.c
+++ b/app/proc-info/main.c
@@ -708,15 +708,20 @@ show_port(void)
 		for (j = 0; j < dev_info.nb_rx_queues; j++) {
 			ret = rte_eth_rx_queue_info_get(i, j, &queue_info);
 			if (ret == 0) {
-				printf("\t  -- queue %d rx scatter %d"
-						" descriptors %d"
-						" offloads 0x%"PRIx64
-						" mempool socket %d\n",
+				printf("\t  -- queue %u rx scatter %u"
+						" descriptors %u"
+						" offloads 0x%" PRIx64
+						" mempool socket %d",
 						j,
 						queue_info.scattered_rx,
 						queue_info.nb_desc,
 						queue_info.conf.offloads,
 						queue_info.mp->socket_id);
+
+				if (queue_info.rx_buf_size != 0)
+					printf(" rx buffer size %u",
+						queue_info.rx_buf_size);
+				printf("\n");
 			}
 		}

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index b6eb2a5..2d9a456 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -452,6 +452,8 @@ rx_queue_infos_display(portid_t port_id, uint16_t queue_id)
 		(qinfo.conf.rx_deferred_start != 0) ? "on" : "off");
 	printf("\nRX scattered packets: %s",
 		(qinfo.scattered_rx != 0) ? "on" : "off");
+	if (qinfo.rx_buf_size != 0)
+		printf("\nRX buffer size: %hu", qinfo.rx_buf_size);
 	printf("\nNumber of RXDs: %hu", qinfo.nb_desc);

 	if (rte_eth_rx_burst_mode_get(port_id, queue_id, &mode) == 0)
diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index b136576..1beb76d 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -189,11 +189,6 @@ Deprecation Notices
   specified lengths into the buffers allocated from the specified
   memory pools. The backward compatibility to existing API is preserved.

-* ethdev: The ``struct rte_eth_rxq_info`` will be modified to include
-  a new optional field, indicating the buffer size used in receiving packets
-  for HW. This change is planned for 20.11. For more details:
-  https://mails.dpdk.org/archives/dev/2020-July/176135.html.
-
 * ethdev: ``rx_descriptor_done`` dev_ops and ``rte_eth_rx_descriptor_done``
   will be removed in 21.11.
   Existing ``rte_eth_rx_descriptor_status`` and ``rte_eth_tx_descriptor_status``
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index c3ab541..eeef52e 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1449,6 +1449,7 @@ struct rte_eth_rxq_info {
 	struct rte_eth_rxconf conf; /**< queue config parameters. */
 	uint8_t scattered_rx;       /**< scattered packets RX supported. */
 	uint16_t nb_desc;           /**< configured number of RXDs. */
+	uint16_t rx_buf_size;       /**< hardware receive buffer size. */
 } __rte_cache_min_aligned;

 /**
--
2.7.4

Re: [dpdk-dev] [PATCH v5 1/2] ethdev: add a field for rxq info structure

[Ferruh Yigit]

On 9/21/2020 2:22 PM, Chengchang Tang wrote:
> Add a field named rx_buf_size in rte_eth_rxq_info to indicate the buffer
> size used in receiving packets for HW.
> 
> In this way, upper-layer users can get this information by calling
> rte_eth_rx_queue_info_get.
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

[dpdk-dev] [PATCH v5 2/2] net/hns3: add Rx buffer size to Rx qinfo query

[Chengchang Tang]

Report hns3 PMD configured Rx buffer size in Rx queue information query.

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
---
 drivers/net/hns3/hns3_rxtx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
index 308d0a6..c13931d 100644
--- a/drivers/net/hns3/hns3_rxtx.c
+++ b/drivers/net/hns3/hns3_rxtx.c
@@ -2851,6 +2851,8 @@ hns3_rxq_info_get(struct rte_eth_dev *dev, uint16_t queue_id,
 	qinfo->mp = rxq->mb_pool;
 	qinfo->nb_desc = rxq->nb_rx_desc;
 	qinfo->scattered_rx = dev->data->scattered_rx;
+	/* Report the HW Rx buffer length to user */
+	qinfo->rx_buf_size = rxq->rx_buf_len;

 	/*
 	 * If there are no available Rx buffer descriptors, incoming packets
--
2.7.4

Re: [dpdk-dev] [PATCH v5 0/2] add Rx buffer size for rxq info structure

[Ferruh Yigit]

On 9/21/2020 2:22 PM, Chengchang Tang wrote:
> In common practice, PMD configure the Rx buffer size which indicate the
> buffer length could be used for HW in receiving packets according to the
> data room size of the object in mempool. But in fact, the final value is
> related to the specifications of HW, and its values will affect the number
> of fragments in receiving packets when scatter is enabled. By the way,
> some PMDs may force to enable scatter when the MTU is bigger than the HW
> Rx buffer size.
> 
> At present, we have no way to expose relevant information to upper layer
> users. So, add a field named rx_buf_size in rte_eth_rxq_info to indicate
> the buffer size used in receiving packets for HW. And this field is
> optional, so there is no need to forcibly update all PMDs.
> 
> This patchset also update testpmd, proc-info tools and add hns3 PMD
> implementation.
> 
> v5:
>    - squash oneline app patches and deprecation remove with ethdev patch
> 
> Chengchang Tang (2):
>    ethdev: add a field for rxq info structure
>    net/hns3: add Rx buffer size to Rx qinfo query
> 

Series applied to dpdk-next-net/main, thanks.