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

[Bing Zhao <bingz@nvidia.com>]

Hi Ori,

> -----Original Message-----
> From: Ori Kam <orika@nvidia.com>
> Sent: Sunday, October 4, 2020 5:22 PM
> To: Bing Zhao <bingz@nvidia.com>; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; ferruh.yigit@intel.com;
> arybchenko@solarflare.com; mdr@ashroe.eu; nhorman@tuxdriver.com;
> bernard.iremonger@intel.com; beilei.xing@intel.com;
> wenzhuo.lu@intel.com
> Cc: dev@dpdk.org
> Subject: RE: [PATCH 2/4] ethdev: add new attributes to hairpin
> config
> 
> 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?

I will remove this and add the description of the modes in the document.

> 
> >  /**
> >   * @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?

By using uint16_t bit fields, there will be some warnings by the compiler and it is not standard.
I prefer to change the "uint16_t peer_count" into "uint32_t peer_count:16" to use the gap before the next structure.
Or yes, I can move it to the end of this current structure.

> Also why do you place the reserved first?

Thanks, I will move it to the end of the u32 like other structure definitions.

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

Thanks