[v3,2/6] ethdev: add new attributes to hairpin config
Checks
Commit Message
To support two ports hairpin mode and keep the backward compatibility
for the application, two new attribute members of hairpin queue
configuration 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 recommended 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 drivers.
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>
Acked-by: Ori Kam <orika@nvidia.com>
---
v2: optimize the structure and remove unused macros
---
lib/librte_ethdev/rte_ethdev.c | 8 ++++----
lib/librte_ethdev/rte_ethdev.h | 5 ++++-
2 files changed, 8 insertions(+), 5 deletions(-)
Comments
08/10/2020 14:05, Bing Zhao:
> struct rte_eth_hairpin_conf {
> - uint16_t peer_count; /**< The number of peers. */
> + uint32_t peer_count:16; /**< The number of peers. */
Why not keeping uint16_t?
> + uint32_t tx_explicit:1; /**< Explicit TX flow rule mode. */
> + uint32_t manual_bind:1; /**< Manually bind hairpin queues. */
Please describe more the expectations of these bits:
What is changed at ethdev or PMD level?
What the application is supposed to do?
Why choosing one mode or the other?
Hi Thomas,
PSB
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, October 13, 2020 5:37 AM
> To: Bing Zhao <bingz@nvidia.com>
> Cc: Ori Kam <orika@nvidia.com>; 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; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 2/6] ethdev: add new attributes to
> hairpin config
>
> External email: Use caution opening links or attachments
>
>
> 08/10/2020 14:05, Bing Zhao:
> > struct rte_eth_hairpin_conf {
> > - uint16_t peer_count; /**< The number of peers. */
> > + uint32_t peer_count:16; /**< The number of peers. */
>
> Why not keeping uint16_t?
The inside structure has a multiple of 4B, and the peer_count now only takes about 2B. AFAIK, usually, the structure will have an aligned length/offset and there will be some padding between the 2B + (2B pad) + 4B * 32 or 2B + (2B +2B) * 32 + 2B, depending on the compiler.
I changed to bit fields of a u32 due to the two facts:
1. Using the 2B and keep the whole structure aligned. No waste except the reserved bits.
2. Only u32 with bit fields is standard.
>
> > + uint32_t tx_explicit:1; /**< Explicit TX flow rule mode. */
> > + uint32_t manual_bind:1; /**< Manually bind hairpin queues.
> */
>
> Please describe more the expectations of these bits:
> What is changed at ethdev or PMD level?
In ethdev level, there is almost no change. This attribute will be passed to PMD directly through the function pointer.
In PMD level, these bits should be checked and better to be saved. And the attribute fields should be checked for per queue pair and all the queues, and each queue pair should have the same attributes configured to make the behavior aligned. But it depends on the PMD itself to decide if all the queue pairs between a port pair should have the same attributes, or even all the queues from / to same port of all hairpin port pairs.
If manual_bind is not set, then the PMD will try to bind the queues and enable hairpin during device start stage and there is no need to call the bind / unbind API.
If tx_explicit is set, the application should insert the RX flow rules and TX flow rules seperataly and connect the RX/TX logic connection together.
> What the application is supposed to do?
The application should specify the new two attributes during the queue setup. And also, it could leave it unset (0 by default) to keep the behavior compatible with the previous release.
If manual_bind is set, then it is the application's responsibility to call the bind / unbind API to enable / disable the hairpin between specific ports.
If tx_explicit is set, as described above, the application should maintain the flows logic to make hairpin work as expected, e.g., they can choose metadata (not the only method), in the RX flow, the metadata is set and in the TX flow, it is used for matching. Then even if the headers are changed with NAT action or encap, the hairpin function could work as expected.
> Why choosing one mode or the other?
If the application wants to have the full control of hairpin flows, it could chose the explicit TX flow mode.
If two or more ports involved into the hairpin, it is suggested to use the manual bind.
Please note, the actual supported attributes denpend on the PMD and HW.
>
Thanks
13/10/2020 14:29, Bing Zhao:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 08/10/2020 14:05, Bing Zhao:
> > > struct rte_eth_hairpin_conf {
> > > - uint16_t peer_count; /**< The number of peers. */
> > > + uint32_t peer_count:16; /**< The number of peers. */
> >
> > Why not keeping uint16_t?
>
> The inside structure has a multiple of 4B, and the peer_count now only takes about 2B. AFAIK, usually, the structure will have an aligned length/offset and there will be some padding between the 2B + (2B pad) + 4B * 32 or 2B + (2B +2B) * 32 + 2B, depending on the compiler.
> I changed to bit fields of a u32 due to the two facts:
> 1. Using the 2B and keep the whole structure aligned. No waste except the reserved bits.
> 2. Only u32 with bit fields is standard.
Oh I see, this is because u16 bit fields are not standard?
> > > + uint32_t tx_explicit:1; /**< Explicit TX flow rule mode. */
> > > + uint32_t manual_bind:1; /**< Manually bind hairpin queues.
> > */
> >
> > Please describe more the expectations of these bits:
> > What is changed at ethdev or PMD level?
>
> In ethdev level, there is almost no change. This attribute will be passed to PMD directly through the function pointer.
> In PMD level, these bits should be checked and better to be saved. And the attribute fields should be checked for per queue pair and all the queues, and each queue pair should have the same attributes configured to make the behavior aligned. But it depends on the PMD itself to decide if all the queue pairs between a port pair should have the same attributes, or even all the queues from / to same port of all hairpin port pairs.
> If manual_bind is not set, then the PMD will try to bind the queues and enable hairpin during device start stage and there is no need to call the bind / unbind API.
> If tx_explicit is set, the application should insert the RX flow rules and TX flow rules seperataly and connect the RX/TX logic connection together.
>
> > What the application is supposed to do?
>
> The application should specify the new two attributes during the queue setup. And also, it could leave it unset (0 by default) to keep the behavior compatible with the previous release.
> If manual_bind is set, then it is the application's responsibility to call the bind / unbind API to enable / disable the hairpin between specific ports.
> If tx_explicit is set, as described above, the application should maintain the flows logic to make hairpin work as expected, e.g., they can choose metadata (not the only method), in the RX flow, the metadata is set and in the TX flow, it is used for matching. Then even if the headers are changed with NAT action or encap, the hairpin function could work as expected.
>
> > Why choosing one mode or the other?
>
> If the application wants to have the full control of hairpin flows, it could chose the explicit TX flow mode.
> If two or more ports involved into the hairpin, it is suggested to use the manual bind.
> Please note, the actual supported attributes denpend on the PMD and HW.
The application impact must be described shortly in doxygen please.
Hi Thomas,
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, October 13, 2020 8:42 PM
> To: Bing Zhao <bingz@nvidia.com>
> Cc: Ori Kam <orika@nvidia.com>; 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; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 2/6] ethdev: add new attributes to
> hairpin config
>
> External email: Use caution opening links or attachments
>
>
> 13/10/2020 14:29, Bing Zhao:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 08/10/2020 14:05, Bing Zhao:
> > > > struct rte_eth_hairpin_conf {
> > > > - uint16_t peer_count; /**< The number of peers. */
> > > > + uint32_t peer_count:16; /**< The number of peers. */
> > >
> > > Why not keeping uint16_t?
> >
> > The inside structure has a multiple of 4B, and the peer_count now
> only takes about 2B. AFAIK, usually, the structure will have an
> aligned length/offset and there will be some padding between the 2B
> + (2B pad) + 4B * 32 or 2B + (2B +2B) * 32 + 2B, depending on the
> compiler.
> > I changed to bit fields of a u32 due to the two facts:
> > 1. Using the 2B and keep the whole structure aligned. No waste
> except the reserved bits.
> > 2. Only u32 with bit fields is standard.
>
> Oh I see, this is because u16 bit fields are not standard?
In some compiler, it is supported. But in the old compiler, it might not and some warning will be raised.
" nonstandard extension used : bit-field types other than int "
"
The following properties of bit fields are implementation-defined:
Whether bit fields of type int are treated as signed or unsigned
Whether types other than int, signed int, unsigned int, and _Bool (since C99) are permitted
"
In C11, it should be supported already.
>
> > > > + uint32_t tx_explicit:1; /**< Explicit TX flow rule mode.
> */
> > > > + uint32_t manual_bind:1; /**< Manually bind hairpin
> queues.
> > > */
> > >
> > > Please describe more the expectations of these bits:
> > > What is changed at ethdev or PMD level?
> >
> > In ethdev level, there is almost no change. This attribute will be
> passed to PMD directly through the function pointer.
> > In PMD level, these bits should be checked and better to be saved.
> And the attribute fields should be checked for per queue pair and
> all the queues, and each queue pair should have the same attributes
> configured to make the behavior aligned. But it depends on the PMD
> itself to decide if all the queue pairs between a port pair should
> have the same attributes, or even all the queues from / to same port
> of all hairpin port pairs.
> > If manual_bind is not set, then the PMD will try to bind the
> queues and enable hairpin during device start stage and there is no
> need to call the bind / unbind API.
> > If tx_explicit is set, the application should insert the RX flow
> rules and TX flow rules separately and connect the RX/TX logic
> connection together.
> >
> > > What the application is supposed to do?
> >
> > The application should specify the new two attributes during the
> queue setup. And also, it could leave it unset (0 by default) to
> keep the behavior compatible with the previous release.
> > If manual_bind is set, then it is the application's responsibility
> to call the bind / unbind API to enable / disable the hairpin
> between specific ports.
> > If tx_explicit is set, as described above, the application should
> maintain the flows logic to make hairpin work as expected, e.g.,
> they can choose metadata (not the only method), in the RX flow, the
> metadata is set and in the TX flow, it is used for matching. Then
> even if the headers are changed with NAT action or encap, the
> hairpin function could work as expected.
> >
> > > Why choosing one mode or the other?
> >
> > If the application wants to have the full control of hairpin flows,
> it could chose the explicit TX flow mode.
> > If two or more ports involved into the hairpin, it is suggested to
> use the manual bind.
> > Please note, the actual supported attributes denpend on the PMD
> and HW.
>
> The application impact must be described shortly in doxygen please.
>
>
OK, sure. I added into the commit message. I will also describe it shortly in the doc.
Thanks
@@ -1954,13 +1954,13 @@ struct rte_eth_dev *
}
if (conf->peer_count > cap.max_rx_2_tx) {
RTE_ETHDEV_LOG(ERR,
- "Invalid value for number of peers for Rx queue(=%hu), should be: <= %hu",
+ "Invalid value for number of peers for Rx queue(=%u), should be: <= %hu",
conf->peer_count, cap.max_rx_2_tx);
return -EINVAL;
}
if (conf->peer_count == 0) {
RTE_ETHDEV_LOG(ERR,
- "Invalid value for number of peers for Rx queue(=%hu), should be: > 0",
+ "Invalid value for number of peers for Rx queue(=%u), should be: > 0",
conf->peer_count);
return -EINVAL;
}
@@ -2125,13 +2125,13 @@ struct rte_eth_dev *
}
if (conf->peer_count > cap.max_tx_2_rx) {
RTE_ETHDEV_LOG(ERR,
- "Invalid value for number of peers for Tx queue(=%hu), should be: <= %hu",
+ "Invalid value for number of peers for Tx queue(=%u), should be: <= %hu",
conf->peer_count, cap.max_tx_2_rx);
return -EINVAL;
}
if (conf->peer_count == 0) {
RTE_ETHDEV_LOG(ERR,
- "Invalid value for number of peers for Tx queue(=%hu), should be: > 0",
+ "Invalid value for number of peers for Tx queue(=%u), should be: > 0",
conf->peer_count);
return -EINVAL;
}
@@ -1045,7 +1045,10 @@ struct rte_eth_hairpin_peer {
* A structure used to configure hairpin binding.
*/
struct rte_eth_hairpin_conf {
- uint16_t peer_count; /**< The number of peers. */
+ uint32_t peer_count:16; /**< The number of peers. */
+ uint32_t tx_explicit:1; /**< Explicit TX flow rule mode. */
+ uint32_t manual_bind:1; /**< Manually bind hairpin queues. */
+ uint32_t reserved:14; /**< Reserved bits. */
struct rte_eth_hairpin_peer peers[RTE_ETH_MAX_HAIRPIN_PEERS];
};