diff mbox series

[2/4] ethdev: add new attributes to hairpin config

Message ID 1601511962-21532-3-git-send-email-bingz@nvidia.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series introduce support for hairpin between two ports |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Bing Zhao Oct. 1, 2020, 12:26 a.m. UTC 
  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(+)

Comments

Ori Kam Oct. 4, 2020, 9:22 a.m. UTC | #1 
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
Bing Zhao Oct. 7, 2020, 11:32 a.m. UTC | #2 
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
diff mbox series

Patch

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)
+
 /**
  * @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. */
 	struct rte_eth_hairpin_peer peers[RTE_ETH_MAX_HAIRPIN_PEERS];
 };