Hi Ori,
> -----Original Message-----
> From: Ori Kam <or...@nvidia.com>
> Sent: Sunday, October 4, 2020 5:22 PM
> To: Bing Zhao <bi...@nvidia.com>; NBU-Contact-Thomas Monjalon
> <tho...@monjalon.net>; ferruh.yi...@intel.com;
> arybche...@solarflare.com; m...@ashroe.eu; nhor...@tuxdriver.com;
> bernard.iremon...@intel.com; beilei.x...@intel.com;
> wenzhuo...@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 <bi...@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 <bi...@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
