Re: [dpdk-dev] [PATCH 2/4] ethdev: add new attributes to hairpin config

[Ori Kam <orika@nvidia.com>]

Hi Bing,

PSB,
Best,
Ori

> -----Original Message-----
> From: Bing Zhao <bingz@nvidia.com>
> Sent: Thursday, October 1, 2020 3:26 AM
> Subject: [PATCH 2/4] ethdev: add new attributes to hairpin config
> 
> To support two ports hairpin mode and keep the backward compatibility
> for the application, two new attribute members of hairpin queue
> config structure are added.
> 
> `tx_explicit` means if the application itself will insert the TX part
> flow rules. If not set, PMD will insert the rules implicitly.
> `manual_bind` means if the hairpin TX queue and peer RX queue will be
> bound automatically during device start stage.
> 
> Different TX and RX queue pairs could have different values, but it
> is highly recommend that all paired queues between one egress and its
> peer ingress ports have the same values, in order not to bring any
> chaos to the system. The actual support of these attribute parameters
> will be checked and decided by the PMD driver.
> 
> In a single port hairpin, if both are zero without any setting, the
> behavior will remain the same as before. It means no bind API needs
> to be called and no TX flow rules need to be inserted manually by
> the application.
> 
> Signed-off-by: Bing Zhao <bingz@nvidia.com>
> ---
>  lib/librte_ethdev/rte_ethdev.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index c3fb684..0cabff0 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1027,6 +1027,21 @@ struct rte_eth_hairpin_cap {
> 
>  #define RTE_ETH_MAX_HAIRPIN_PEERS 32
> 
> +/*
> + * Hairpin queue attribute parameters.
> + * Each TX queue and peer RX queue should have the same value.
> + * Default value 0 is for backward-compatibility, the same behaviors should
> + * remain if the value is not set (0).
> + */
> +/**< Hairpin queues will be bound automatically */
> +#define RTE_ETH_HAIRPIN_BIND_AUTO		(0)
> +/**< Hairpin queues will be bound manually with bind API */
> +#define RTE_ETH_HAIRPIN_BIND_MANUAL		(1)
> +/**< Hairpin TX part flow rule will be inserted implicitly by PMD */
> +#define RTE_ETH_HAIRPIN_TXRULE_IMPLICIT		(0)
> +/**< Hairpin TX part flow rule will be inserted explicitly by APP */
> +#define RTE_ETH_HAIRPIN_TXRULE_EXPLICIT		(1)
> +

Why do you need those defines if you are using bit fields?

>  /**
>   * @warning
>   * @b EXPERIMENTAL: this API may change, or be removed, without prior
> notice
> @@ -1046,6 +1061,9 @@ struct rte_eth_hairpin_peer {
>   */
>  struct rte_eth_hairpin_conf {
>  	uint16_t peer_count; /**< The number of peers. */
> +	uint32_t reserved : 30; /**< Reserved bits. */
> +	uint32_t tx_explicit : 1; /**< Explicit TX flow rule mode. */
> +	uint32_t manual_bind : 1; /**< Manually bind hairpin queues. */

Why not place the new bits at the end?
Also why do you place the reserved first?

>  	struct rte_eth_hairpin_peer peers[RTE_ETH_MAX_HAIRPIN_PEERS];
>  };
> 
> --
> 2.5.5