[dpdk-dev] [PATCH] reta_query: Doc requirements on reta_conf

[dpdk-dev] [PATCH] reta_query: Doc requirements on reta_conf

[Tom Barbette]

As the librte function checks that bits of up to reta_size in reta_conf
are set, the arg passed must have the mask set to 1. I spent quite some
time before finding the issue here, so I thought updating the doc may
help other people.

The problem raise when the size of the table is not on a (byte?)
boundary, eg when using a power-of-2 amount of queues with mlx5, the
table size becomes the number of queue. Then a usual (bzero)
initialization raise EINVAL, because of this bits problem.

I'd say the requirement should go, one expects this functions to overwrite
whatever is passed as table arguments. Who would want to query the table
except a few entries?
Maybe (idk) mlx5 is actually the only device to support non-byte sized RSS
table, and mlx5 actually ignore the mask in reta_query...
---
 lib/librte_ethdev/rte_ethdev.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index e254da8..ace425f 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -2941,7 +2941,7 @@ int rte_eth_dev_rss_reta_update(uint16_t port_id,
  * @param port_id
  *   The port identifier of the Ethernet device.
  * @param reta_conf
- *   RETA to query.
+ *   RETA to query. The mask bits must be set according to reta_size.
  * @param reta_size
  *   Redirection table size. The table size can be queried by
  *   rte_eth_dev_info_get().
-- 
2.7.4

[dpdk-dev] [PATCH] reta_query: Doc requirements on reta_conf

[Tom Barbette]

As the librte function checks that bits of up to reta_size in reta_conf
are set, the arg passed must have the mask set to 1. I spent quite some
time before finding the issue here, so I thought updating the doc may
help other people.

The problem raise when the size of the table is not on a (byte?)
boundary, eg when using a power-of-2 amount of queues with mlx5, the
table size becomes the number of queue. Then a usual (bzero)
initialization raise EINVAL, because of this bits problem.

I'd say the requirement should go, one expects this functions to overwrite
whatever is passed as table arguments. Who would want to query the table
except a few entries?
Maybe (idk) mlx5 is actually the only device to support non-byte sized RSS
table, and mlx5 actually ignore the mask in reta_query...
---
 lib/librte_ethdev/rte_ethdev.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index e254da8..ace425f 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -2941,7 +2941,7 @@ int rte_eth_dev_rss_reta_update(uint16_t port_id,
  * @param port_id
  *   The port identifier of the Ethernet device.
  * @param reta_conf
- *   RETA to query.
+ *   RETA to query. The mask bits must be set according to reta_size.
  * @param reta_size
  *   Redirection table size. The table size can be queried by
  *   rte_eth_dev_info_get().
-- 
2.7.4

Re: [dpdk-dev] [PATCH] reta_query: Doc requirements on reta_conf

[Ferruh Yigit]

On 4/3/2019 9:48 AM, Tom Barbette wrote:
> As the librte function checks that bits of up to reta_size in reta_conf
> are set, the arg passed must have the mask set to 1. I spent quite some
> time before finding the issue here, so I thought updating the doc may
> help other people.

In 'reta_conf' there are 64 table entries and have a 64 bits 'mask', for each
entry requested matching mask bit should be set.

'rte_eth_dev_rss_reta_query()' returns error if none of the bits in 'mask' set,
this makes sense because it means user is calling this function without
requesting any work at all.

So the requirement is not to set 'mask' to 1 but set bits per requested table entry.

> 
> The problem raise when the size of the table is not on a (byte?)
> boundary, eg when using a power-of-2 amount of queues with mlx5, the
> table size becomes the number of queue. Then a usual (bzero)
> initialization raise EINVAL, because of this bits problem.
> 
> I'd say the requirement should go, one expects this functions to overwrite
> whatever is passed as table arguments. Who would want to query the table
> except a few entries?> Maybe (idk) mlx5 is actually the only device to support non-byte sized RSS
> table, and mlx5 actually ignore the mask in reta_query...

You are right, mlx5 implementation is wrong!

It should take account the mask and set only values in the reta that requested.
Although returning all table will work from user point of view since values user
looking for will be valid in the table. But it is wrong from API implementation
point of view.

> ---
>  lib/librte_ethdev/rte_ethdev.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index e254da8..ace425f 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -2941,7 +2941,7 @@ int rte_eth_dev_rss_reta_update(uint16_t port_id,
>   * @param port_id
>   *   The port identifier of the Ethernet device.
>   * @param reta_conf
> - *   RETA to query.
> + *   RETA to query. The mask bits must be set according to reta_size.

Overall no objection to clarify mask bits should be set requirement, but not
sure if it should be documented related to reta_size.

what about something like following, please feel free to update/correct as needed:
'For each requested reta entry, corresponding bit in mask must be set.'

>   * @param reta_size
>   *   Redirection table size. The table size can be queried by
>   *   rte_eth_dev_info_get().
> 

Re: [dpdk-dev] [PATCH] reta_query: Doc requirements on reta_conf

[Ferruh Yigit]

On 4/3/2019 9:48 AM, Tom Barbette wrote:
> As the librte function checks that bits of up to reta_size in reta_conf
> are set, the arg passed must have the mask set to 1. I spent quite some
> time before finding the issue here, so I thought updating the doc may
> help other people.

In 'reta_conf' there are 64 table entries and have a 64 bits 'mask', for each
entry requested matching mask bit should be set.

'rte_eth_dev_rss_reta_query()' returns error if none of the bits in 'mask' set,
this makes sense because it means user is calling this function without
requesting any work at all.

So the requirement is not to set 'mask' to 1 but set bits per requested table entry.

> 
> The problem raise when the size of the table is not on a (byte?)
> boundary, eg when using a power-of-2 amount of queues with mlx5, the
> table size becomes the number of queue. Then a usual (bzero)
> initialization raise EINVAL, because of this bits problem.
> 
> I'd say the requirement should go, one expects this functions to overwrite
> whatever is passed as table arguments. Who would want to query the table
> except a few entries?> Maybe (idk) mlx5 is actually the only device to support non-byte sized RSS
> table, and mlx5 actually ignore the mask in reta_query...

You are right, mlx5 implementation is wrong!

It should take account the mask and set only values in the reta that requested.
Although returning all table will work from user point of view since values user
looking for will be valid in the table. But it is wrong from API implementation
point of view.

> ---
>  lib/librte_ethdev/rte_ethdev.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index e254da8..ace425f 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -2941,7 +2941,7 @@ int rte_eth_dev_rss_reta_update(uint16_t port_id,
>   * @param port_id
>   *   The port identifier of the Ethernet device.
>   * @param reta_conf
> - *   RETA to query.
> + *   RETA to query. The mask bits must be set according to reta_size.

Overall no objection to clarify mask bits should be set requirement, but not
sure if it should be documented related to reta_size.

what about something like following, please feel free to update/correct as needed:
'For each requested reta entry, corresponding bit in mask must be set.'

>   * @param reta_size
>   *   Redirection table size. The table size can be queried by
>   *   rte_eth_dev_info_get().
>