net: introduce IPv4 ihl and version fields
Checks
Commit Message
RTE IPv4 header definition combines the `version' and `ihl' fields
into a single structure member.
This patch introduces dedicated structure members for both `version'
and `ihl' IPv4 fields. Separated header fields definitions allow to
create simplified code to match on the IHL value in a flow rule.
The original `version_ihl' structure member is kept for backward
compatibility.
Signed-off-by: Gregory Etelson <getelson@nvidia.com>
---
app/test/test_flow_classify.c | 8 ++++----
lib/net/rte_ip.h | 16 +++++++++++++++-
2 files changed, 19 insertions(+), 5 deletions(-)
Comments
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Gregory Etelson
> Sent: Thursday, 27 May 2021 17.29
and version fields
>
> RTE IPv4 header definition combines the `version' and `ihl' fields
> into a single structure member.
> This patch introduces dedicated structure members for both `version'
> and `ihl' IPv4 fields. Separated header fields definitions allow to
> create simplified code to match on the IHL value in a flow rule.
> The original `version_ihl' structure member is kept for backward
> compatibility.
>
> Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> ---
> app/test/test_flow_classify.c | 8 ++++----
> lib/net/rte_ip.h | 16 +++++++++++++++-
> 2 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/app/test/test_flow_classify.c
> b/app/test/test_flow_classify.c
> index 951606f248..4f64be5357 100644
> --- a/app/test/test_flow_classify.c
> +++ b/app/test/test_flow_classify.c
> @@ -95,7 +95,7 @@ static struct rte_acl_field_def
> ipv4_defs[NUM_FIELDS_IPV4] = {
> * dst mask 255.255.255.00 / udp src is 32 dst is 33 / end"
> */
> static struct rte_flow_item_ipv4 ipv4_udp_spec_1 = {
> - { 0, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
> + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
> RTE_IPV4(2, 2, 2, 3), RTE_IPV4(2, 2, 2, 7)}
> };
> static const struct rte_flow_item_ipv4 ipv4_mask_24 = {
> @@ -131,7 +131,7 @@ static struct rte_flow_item end_item = {
> RTE_FLOW_ITEM_TYPE_END,
> * dst mask 255.255.255.00 / tcp src is 16 dst is 17 / end"
> */
> static struct rte_flow_item_ipv4 ipv4_tcp_spec_1 = {
> - { 0, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
> + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
> RTE_IPV4(1, 2, 3, 4), RTE_IPV4(5, 6, 7, 8)}
> };
>
> @@ -150,8 +150,8 @@ static struct rte_flow_item tcp_item_1 = {
> RTE_FLOW_ITEM_TYPE_TCP,
> * dst mask 255.255.255.00 / sctp src is 16 dst is 17/ end"
> */
> static struct rte_flow_item_ipv4 ipv4_sctp_spec_1 = {
> - { 0, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0, RTE_IPV4(11, 12, 13, 14),
> - RTE_IPV4(15, 16, 17, 18)}
> + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0,
> + RTE_IPV4(11, 12, 13, 14), RTE_IPV4(15, 16, 17, 18)}
> };
>
> static struct rte_flow_item_sctp sctp_spec_1 = {
> diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
> index 4b728969c1..684bb028b2 100644
> --- a/lib/net/rte_ip.h
> +++ b/lib/net/rte_ip.h
> @@ -38,7 +38,21 @@ extern "C" {
> * IPv4 Header
> */
> struct rte_ipv4_hdr {
> - uint8_t version_ihl; /**< version and header length */
> + __extension__
> + union {
> + uint8_t version_ihl; /**< version and header length */
> + struct {
> +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> + uint8_t ihl:4;
> + uint8_t version:4;
> +#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> + uint8_t version:4;
> + uint8_t ihl:4;
> +#else
> +#error "setup endian definition"
> +#endif
> + };
> + };
> uint8_t type_of_service; /**< type of service */
> rte_be16_t total_length; /**< length of packet */
> rte_be16_t packet_id; /**< packet ID */
> --
> 2.31.1
>
This does not break the ABI, but it could be discussed if it breaks the API due to the required structure initialization changes shown in test_flow_classify.c. I think this patch is an improvement, and that such structure modifications should be generally accepted, so:
Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Gregory Etelson
> > Sent: Thursday, 27 May 2021 17.29
> and version fields
> >
> > RTE IPv4 header definition combines the `version' and `ihl' fields
> > into a single structure member.
> > This patch introduces dedicated structure members for both `version'
> > and `ihl' IPv4 fields. Separated header fields definitions allow to
> > create simplified code to match on the IHL value in a flow rule.
> > The original `version_ihl' structure member is kept for backward
> > compatibility.
> >
> > Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> > ---
> > app/test/test_flow_classify.c | 8 ++++----
> > lib/net/rte_ip.h | 16 +++++++++++++++-
> > 2 files changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/app/test/test_flow_classify.c
> > b/app/test/test_flow_classify.c
> > index 951606f248..4f64be5357 100644
> > --- a/app/test/test_flow_classify.c
> > +++ b/app/test/test_flow_classify.c
> > @@ -95,7 +95,7 @@ static struct rte_acl_field_def
> > ipv4_defs[NUM_FIELDS_IPV4] = {
> > * dst mask 255.255.255.00 / udp src is 32 dst is 33 / end"
> > */
> > static struct rte_flow_item_ipv4 ipv4_udp_spec_1 = {
> > - { 0, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
> > + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
> > RTE_IPV4(2, 2, 2, 3), RTE_IPV4(2, 2, 2, 7)}
> > };
> > static const struct rte_flow_item_ipv4 ipv4_mask_24 = {
> > @@ -131,7 +131,7 @@ static struct rte_flow_item end_item = {
> > RTE_FLOW_ITEM_TYPE_END,
> > * dst mask 255.255.255.00 / tcp src is 16 dst is 17 / end"
> > */
> > static struct rte_flow_item_ipv4 ipv4_tcp_spec_1 = {
> > - { 0, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
> > + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
> > RTE_IPV4(1, 2, 3, 4), RTE_IPV4(5, 6, 7, 8)}
> > };
> >
> > @@ -150,8 +150,8 @@ static struct rte_flow_item tcp_item_1 = {
> > RTE_FLOW_ITEM_TYPE_TCP,
> > * dst mask 255.255.255.00 / sctp src is 16 dst is 17/ end"
> > */
> > static struct rte_flow_item_ipv4 ipv4_sctp_spec_1 = {
> > - { 0, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0, RTE_IPV4(11, 12, 13, 14),
> > - RTE_IPV4(15, 16, 17, 18)}
> > + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0,
> > + RTE_IPV4(11, 12, 13, 14), RTE_IPV4(15, 16, 17, 18)}
> > };
> >
> > static struct rte_flow_item_sctp sctp_spec_1 = {
> > diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
> > index 4b728969c1..684bb028b2 100644
> > --- a/lib/net/rte_ip.h
> > +++ b/lib/net/rte_ip.h
> > @@ -38,7 +38,21 @@ extern "C" {
> > * IPv4 Header
> > */
> > struct rte_ipv4_hdr {
> > - uint8_t version_ihl; /**< version and header length */
> > + __extension__
> > + union {
> > + uint8_t version_ihl; /**< version and header length */
> > + struct {
> > +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> > + uint8_t ihl:4;
> > + uint8_t version:4;
> > +#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> > + uint8_t version:4;
> > + uint8_t ihl:4;
> > +#else
> > +#error "setup endian definition"
> > +#endif
> > + };
> > + };
> > uint8_t type_of_service; /**< type of service */
> > rte_be16_t total_length; /**< length of packet */
> > rte_be16_t packet_id; /**< packet ID */
> > --
> > 2.31.1
> >
>
> This does not break the ABI, but it could be discussed if it breaks the API due to the required structure initialization changes shown in
> test_flow_classify.c.
Yep, I guess it might be classified as API change.
Another thing that concerns me - it is not the only place in IPv4 header when we unite multiple bit-fields into one field:
type_of_service, fragment_offset.
If we start splitting ipv4 fields into actual bitfields, I suppose we'll end-up splitting these ones too.
But I am not sure it will pay off - as compiler not always generates optimal code for reading/updating bitfields.
Did you consider just adding extra macros to simplify access to these fields (like RTE_IPV4_HDR_(GET_SET)_*),
instead?
> I think this patch is an improvement, and that such structure modifications should be generally accepted, so:
>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> Konstantin
> Sent: Friday, 28 May 2021 12.21
>
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Gregory
> Etelson
> > > Sent: Thursday, 27 May 2021 17.29
> > and version fields
> > >
> > > RTE IPv4 header definition combines the `version' and `ihl' fields
> > > into a single structure member.
> > > This patch introduces dedicated structure members for both
> `version'
> > > and `ihl' IPv4 fields. Separated header fields definitions allow to
> > > create simplified code to match on the IHL value in a flow rule.
> > > The original `version_ihl' structure member is kept for backward
> > > compatibility.
> > >
> > > Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> > > ---
> > > app/test/test_flow_classify.c | 8 ++++----
> > > lib/net/rte_ip.h | 16 +++++++++++++++-
> > > 2 files changed, 19 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/app/test/test_flow_classify.c
> > > b/app/test/test_flow_classify.c
> > > index 951606f248..4f64be5357 100644
> > > --- a/app/test/test_flow_classify.c
> > > +++ b/app/test/test_flow_classify.c
> > > @@ -95,7 +95,7 @@ static struct rte_acl_field_def
> > > ipv4_defs[NUM_FIELDS_IPV4] = {
> > > * dst mask 255.255.255.00 / udp src is 32 dst is 33 / end"
> > > */
> > > static struct rte_flow_item_ipv4 ipv4_udp_spec_1 = {
> > > - { 0, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
> > > + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
> > > RTE_IPV4(2, 2, 2, 3), RTE_IPV4(2, 2, 2, 7)}
> > > };
> > > static const struct rte_flow_item_ipv4 ipv4_mask_24 = {
> > > @@ -131,7 +131,7 @@ static struct rte_flow_item end_item = {
> > > RTE_FLOW_ITEM_TYPE_END,
> > > * dst mask 255.255.255.00 / tcp src is 16 dst is 17 / end"
> > > */
> > > static struct rte_flow_item_ipv4 ipv4_tcp_spec_1 = {
> > > - { 0, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
> > > + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
> > > RTE_IPV4(1, 2, 3, 4), RTE_IPV4(5, 6, 7, 8)}
> > > };
> > >
> > > @@ -150,8 +150,8 @@ static struct rte_flow_item tcp_item_1 = {
> > > RTE_FLOW_ITEM_TYPE_TCP,
> > > * dst mask 255.255.255.00 / sctp src is 16 dst is 17/ end"
> > > */
> > > static struct rte_flow_item_ipv4 ipv4_sctp_spec_1 = {
> > > - { 0, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0, RTE_IPV4(11, 12, 13, 14),
> > > - RTE_IPV4(15, 16, 17, 18)}
> > > + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0,
> > > + RTE_IPV4(11, 12, 13, 14), RTE_IPV4(15, 16, 17, 18)}
> > > };
> > >
> > > static struct rte_flow_item_sctp sctp_spec_1 = {
> > > diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
> > > index 4b728969c1..684bb028b2 100644
> > > --- a/lib/net/rte_ip.h
> > > +++ b/lib/net/rte_ip.h
> > > @@ -38,7 +38,21 @@ extern "C" {
> > > * IPv4 Header
> > > */
> > > struct rte_ipv4_hdr {
> > > - uint8_t version_ihl; /**< version and header length */
> > > + __extension__
> > > + union {
> > > + uint8_t version_ihl; /**< version and header length */
> > > + struct {
> > > +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> > > + uint8_t ihl:4;
> > > + uint8_t version:4;
> > > +#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> > > + uint8_t version:4;
> > > + uint8_t ihl:4;
> > > +#else
> > > +#error "setup endian definition"
> > > +#endif
> > > + };
> > > + };
> > > uint8_t type_of_service; /**< type of service */
> > > rte_be16_t total_length; /**< length of packet */
> > > rte_be16_t packet_id; /**< packet ID */
> > > --
> > > 2.31.1
> > >
> >
> > This does not break the ABI, but it could be discussed if it breaks
> the API due to the required structure initialization changes shown in
> > test_flow_classify.c.
>
> Yep, I guess it might be classified as API change.
> Another thing that concerns me - it is not the only place in IPv4
> header when we unite multiple bit-fields into one field:
> type_of_service, fragment_offset.
> If we start splitting ipv4 fields into actual bitfields, I suppose
> we'll end-up splitting these ones too.
> But I am not sure it will pay off - as compiler not always generates
> optimal code for reading/updating bitfields.
> Did you consider just adding extra macros to simplify access to these
> fields (like RTE_IPV4_HDR_(GET_SET)_*),
> instead?
>
Let's please not introduce accessor macros for bitfields. If we don't introduce bitfields like these, I would rather stick with the current _MASK, _SHIFT and _FLAG defines.
Yes, this change will lead to the introduction of more bitfields, both here and in other places. We already accepted it in the eCPRI structure (/lib/net/rte_ecpri.h), so why not just generally accept it.
Are modern compilers really worse at handling a bitfield defined like this, compared to handling a single uint8_t with hand coding? I consider your concern very important, so I'm only asking if it is still relevant, to avoid making decisions based on past experience that might be outdated. (I admit to falling into that trap myself, once in a while.)
> > I think this patch is an improvement, and that such structure
> modifications should be generally accepted, so:
> >
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
>
> > > > RTE IPv4 header definition combines the `version' and `ihl'
> > > > fields into a single structure member.
> > > > This patch introduces dedicated structure members for both
> > `version'
> > > > and `ihl' IPv4 fields. Separated header fields definitions allow
> > > > to create simplified code to match on the IHL value in a flow rule.
> > > > The original `version_ihl' structure member is kept for backward
> > > > compatibility.
> > > >
> > > > Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> > > > ---
> > > > app/test/test_flow_classify.c | 8 ++++----
> > > > lib/net/rte_ip.h | 16 +++++++++++++++-
> > > > 2 files changed, 19 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/app/test/test_flow_classify.c
> > > > b/app/test/test_flow_classify.c index 951606f248..4f64be5357
> > > > 100644
> > > > --- a/app/test/test_flow_classify.c
> > > > +++ b/app/test/test_flow_classify.c
> > > > @@ -95,7 +95,7 @@ static struct rte_acl_field_def
> > > > ipv4_defs[NUM_FIELDS_IPV4] = {
> > > > * dst mask 255.255.255.00 / udp src is 32 dst is 33 / end"
> > > > */
> > > > static struct rte_flow_item_ipv4 ipv4_udp_spec_1 = {
> > > > - { 0, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
> > > > + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
> > > > RTE_IPV4(2, 2, 2, 3), RTE_IPV4(2, 2, 2, 7)} }; static const
> > > > struct rte_flow_item_ipv4 ipv4_mask_24 = { @@ -131,7 +131,7 @@
> > > > static struct rte_flow_item end_item = {
> RTE_FLOW_ITEM_TYPE_END,
> > > > * dst mask 255.255.255.00 / tcp src is 16 dst is 17 / end"
> > > > */
> > > > static struct rte_flow_item_ipv4 ipv4_tcp_spec_1 = {
> > > > - { 0, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
> > > > + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
> > > > RTE_IPV4(1, 2, 3, 4), RTE_IPV4(5, 6, 7, 8)} };
> > > >
> > > > @@ -150,8 +150,8 @@ static struct rte_flow_item tcp_item_1 = {
> > > > RTE_FLOW_ITEM_TYPE_TCP,
> > > > * dst mask 255.255.255.00 / sctp src is 16 dst is 17/ end"
> > > > */
> > > > static struct rte_flow_item_ipv4 ipv4_sctp_spec_1 = {
> > > > - { 0, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0, RTE_IPV4(11, 12, 13, 14),
> > > > - RTE_IPV4(15, 16, 17, 18)}
> > > > + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0,
> > > > + RTE_IPV4(11, 12, 13, 14), RTE_IPV4(15, 16, 17, 18)}
> > > > };
> > > >
> > > > static struct rte_flow_item_sctp sctp_spec_1 = { diff --git
> > > > a/lib/net/rte_ip.h b/lib/net/rte_ip.h index 4b728969c1..684bb028b2
> > > > 100644
> > > > --- a/lib/net/rte_ip.h
> > > > +++ b/lib/net/rte_ip.h
> > > > @@ -38,7 +38,21 @@ extern "C" {
> > > > * IPv4 Header
> > > > */
> > > > struct rte_ipv4_hdr {
> > > > - uint8_t version_ihl; /**< version and header length */
> > > > + __extension__
> > > > + union {
> > > > + uint8_t version_ihl; /**< version and header length */
> > > > + struct {
> > > > +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> > > > + uint8_t ihl:4;
> > > > + uint8_t version:4; #elif RTE_BYTE_ORDER ==
> > > > +RTE_BIG_ENDIAN
> > > > + uint8_t version:4;
> > > > + uint8_t ihl:4;
> > > > +#else
> > > > +#error "setup endian definition"
> > > > +#endif
> > > > + };
> > > > + };
> > > > uint8_t type_of_service; /**< type of service */
> > > > rte_be16_t total_length; /**< length of packet */
> > > > rte_be16_t packet_id; /**< packet ID */
> > > > --
> > > > 2.31.1
> > > >
> > >
> > > This does not break the ABI, but it could be discussed if it breaks
> > the API due to the required structure initialization changes shown in
> > > test_flow_classify.c.
> >
> > Yep, I guess it might be classified as API change.
> > Another thing that concerns me - it is not the only place in IPv4
> > header when we unite multiple bit-fields into one field:
> > type_of_service, fragment_offset.
> > If we start splitting ipv4 fields into actual bitfields, I suppose
> > we'll end-up splitting these ones too.
> > But I am not sure it will pay off - as compiler not always generates
> > optimal code for reading/updating bitfields.
> > Did you consider just adding extra macros to simplify access to these
> > fields (like RTE_IPV4_HDR_(GET_SET)_*), instead?
> >
>
> Let's please not introduce accessor macros for bitfields. If we don't
> introduce bitfields like these, I would rather stick with the current _MASK,
> _SHIFT and _FLAG defines.
>
> Yes, this change will lead to the introduction of more bitfields, both here
> and in other places. We already accepted it in the eCPRI structure
> (/lib/net/rte_ecpri.h), so why not just generally accept it.
>
> Are modern compilers really worse at handling a bitfield defined like this,
> compared to handling a single uint8_t with hand coding? I consider your
> concern very important, so I'm only asking if it is still relevant, to avoid
> making decisions based on past experience that might be outdated. (I admit
> to falling into that trap myself, once in a while.)
>
I compared x86 code generated with gcc-9, gcc-10 and clang-10 for these 2 functions:
void test_ipv4_hdr_byte(struct rte_ipv4_hdr *h, uint8_t version, uint8_t ihl)
{
h->version_ihl = ((version & 0x0f) << 4) | (ihl & 0x0f);
}
void test_ipv4_hdr_bits(struct rte_ipv4_hdr *h, uint8_t version, uint8_t ihl)
{
h->version = version & 0x0f;
h->ihl = ihl & 0x0f;
}
meson configuration flags: --default-library=static --buildtype=release
Each compiler produced identical code for both functions.
> > > I think this patch is an improvement, and that such structure
> > modifications should be generally accepted, so:
> > >
> > > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> >
> > > > > RTE IPv4 header definition combines the `version' and `ihl'
> > > > > fields into a single structure member.
> > > > > This patch introduces dedicated structure members for both
> > > `version'
> > > > > and `ihl' IPv4 fields. Separated header fields definitions allow
> > > > > to create simplified code to match on the IHL value in a flow rule.
> > > > > The original `version_ihl' structure member is kept for backward
> > > > > compatibility.
> > > > >
> > > > > Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> > > > > ---
> > > > > app/test/test_flow_classify.c | 8 ++++----
> > > > > lib/net/rte_ip.h | 16 +++++++++++++++-
> > > > > 2 files changed, 19 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/app/test/test_flow_classify.c
> > > > > b/app/test/test_flow_classify.c index 951606f248..4f64be5357
> > > > > 100644
> > > > > --- a/app/test/test_flow_classify.c
> > > > > +++ b/app/test/test_flow_classify.c
> > > > > @@ -95,7 +95,7 @@ static struct rte_acl_field_def
> > > > > ipv4_defs[NUM_FIELDS_IPV4] = {
> > > > > * dst mask 255.255.255.00 / udp src is 32 dst is 33 / end"
> > > > > */
> > > > > static struct rte_flow_item_ipv4 ipv4_udp_spec_1 = {
> > > > > - { 0, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
> > > > > + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
> > > > > RTE_IPV4(2, 2, 2, 3), RTE_IPV4(2, 2, 2, 7)} }; static const
> > > > > struct rte_flow_item_ipv4 ipv4_mask_24 = { @@ -131,7 +131,7 @@
> > > > > static struct rte_flow_item end_item = {
> > RTE_FLOW_ITEM_TYPE_END,
> > > > > * dst mask 255.255.255.00 / tcp src is 16 dst is 17 / end"
> > > > > */
> > > > > static struct rte_flow_item_ipv4 ipv4_tcp_spec_1 = {
> > > > > - { 0, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
> > > > > + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
> > > > > RTE_IPV4(1, 2, 3, 4), RTE_IPV4(5, 6, 7, 8)} };
> > > > >
> > > > > @@ -150,8 +150,8 @@ static struct rte_flow_item tcp_item_1 = {
> > > > > RTE_FLOW_ITEM_TYPE_TCP,
> > > > > * dst mask 255.255.255.00 / sctp src is 16 dst is 17/ end"
> > > > > */
> > > > > static struct rte_flow_item_ipv4 ipv4_sctp_spec_1 = {
> > > > > - { 0, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0, RTE_IPV4(11, 12, 13, 14),
> > > > > - RTE_IPV4(15, 16, 17, 18)}
> > > > > + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0,
> > > > > + RTE_IPV4(11, 12, 13, 14), RTE_IPV4(15, 16, 17, 18)}
> > > > > };
> > > > >
> > > > > static struct rte_flow_item_sctp sctp_spec_1 = { diff --git
> > > > > a/lib/net/rte_ip.h b/lib/net/rte_ip.h index 4b728969c1..684bb028b2
> > > > > 100644
> > > > > --- a/lib/net/rte_ip.h
> > > > > +++ b/lib/net/rte_ip.h
> > > > > @@ -38,7 +38,21 @@ extern "C" {
> > > > > * IPv4 Header
> > > > > */
> > > > > struct rte_ipv4_hdr {
> > > > > - uint8_t version_ihl; /**< version and header length */
> > > > > + __extension__
> > > > > + union {
> > > > > + uint8_t version_ihl; /**< version and header length */
> > > > > + struct {
> > > > > +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> > > > > + uint8_t ihl:4;
> > > > > + uint8_t version:4; #elif RTE_BYTE_ORDER ==
> > > > > +RTE_BIG_ENDIAN
> > > > > + uint8_t version:4;
> > > > > + uint8_t ihl:4;
> > > > > +#else
> > > > > +#error "setup endian definition"
> > > > > +#endif
> > > > > + };
> > > > > + };
> > > > > uint8_t type_of_service; /**< type of service */
> > > > > rte_be16_t total_length; /**< length of packet */
> > > > > rte_be16_t packet_id; /**< packet ID */
> > > > > --
> > > > > 2.31.1
> > > > >
> > > >
> > > > This does not break the ABI, but it could be discussed if it breaks
> > > the API due to the required structure initialization changes shown in
> > > > test_flow_classify.c.
> > >
> > > Yep, I guess it might be classified as API change.
> > > Another thing that concerns me - it is not the only place in IPv4
> > > header when we unite multiple bit-fields into one field:
> > > type_of_service, fragment_offset.
> > > If we start splitting ipv4 fields into actual bitfields, I suppose
> > > we'll end-up splitting these ones too.
> > > But I am not sure it will pay off - as compiler not always generates
> > > optimal code for reading/updating bitfields.
> > > Did you consider just adding extra macros to simplify access to these
> > > fields (like RTE_IPV4_HDR_(GET_SET)_*), instead?
> > >
> >
> > Let's please not introduce accessor macros for bitfields. If we don't
> > introduce bitfields like these, I would rather stick with the current _MASK,
> > _SHIFT and _FLAG defines.
> >
> > Yes, this change will lead to the introduction of more bitfields, both here
> > and in other places. We already accepted it in the eCPRI structure
> > (/lib/net/rte_ecpri.h), so why not just generally accept it.
> >
> > Are modern compilers really worse at handling a bitfield defined like this,
> > compared to handling a single uint8_t with hand coding? I consider your
> > concern very important, so I'm only asking if it is still relevant, to avoid
> > making decisions based on past experience that might be outdated. (I admit
> > to falling into that trap myself, once in a while.)
> >
>
> I compared x86 code generated with gcc-9, gcc-10 and clang-10 for these 2 functions:
> void test_ipv4_hdr_byte(struct rte_ipv4_hdr *h, uint8_t version, uint8_t ihl)
> {
> h->version_ihl = ((version & 0x0f) << 4) | (ihl & 0x0f);
> }
> void test_ipv4_hdr_bits(struct rte_ipv4_hdr *h, uint8_t version, uint8_t ihl)
> {
> h->version = version & 0x0f;
> h->ihl = ihl & 0x0f;
> }
> meson configuration flags: --default-library=static --buildtype=release
> Each compiler produced identical code for both functions.
For that particular case (2 bit-fields packed tightly into one byte)
compilers usually perform quite well. At least I never saw issues for such case.
Bit-fields that do cross byte boundaries - that might be a trouble.
>
>
> > > > I think this patch is an improvement, and that such structure
> > > modifications should be generally accepted, so:
> > > >
> > > > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > >
> > > > > > RTE IPv4 header definition combines the `version' and `ihl'
> > > > > > fields into a single structure member.
> > > > > > This patch introduces dedicated structure members for both
> > > > `version'
> > > > > > and `ihl' IPv4 fields. Separated header fields definitions
> > > > > > allow to create simplified code to match on the IHL value in a flow
> rule.
> > > > > > The original `version_ihl' structure member is kept for
> > > > > > backward compatibility.
> > > > > >
> > > > > > Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> > > > > > ---
> > > > > > app/test/test_flow_classify.c | 8 ++++----
> > > > > > lib/net/rte_ip.h | 16 +++++++++++++++-
> > > > > > 2 files changed, 19 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git a/app/test/test_flow_classify.c
> > > > > > b/app/test/test_flow_classify.c index 951606f248..4f64be5357
> > > > > > 100644
> > > > > > --- a/app/test/test_flow_classify.c
> > > > > > +++ b/app/test/test_flow_classify.c
> > > > > > @@ -95,7 +95,7 @@ static struct rte_acl_field_def
> > > > > > ipv4_defs[NUM_FIELDS_IPV4] = {
> > > > > > * dst mask 255.255.255.00 / udp src is 32 dst is 33 / end"
> > > > > > */
> > > > > > static struct rte_flow_item_ipv4 ipv4_udp_spec_1 = {
> > > > > > - { 0, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
> > > > > > + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
> > > > > > RTE_IPV4(2, 2, 2, 3), RTE_IPV4(2, 2, 2, 7)} }; static
> > > > > > const struct rte_flow_item_ipv4 ipv4_mask_24 = { @@ -131,7
> > > > > > +131,7 @@ static struct rte_flow_item end_item = {
> > > RTE_FLOW_ITEM_TYPE_END,
> > > > > > * dst mask 255.255.255.00 / tcp src is 16 dst is 17 / end"
> > > > > > */
> > > > > > static struct rte_flow_item_ipv4 ipv4_tcp_spec_1 = {
> > > > > > - { 0, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
> > > > > > + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
> > > > > > RTE_IPV4(1, 2, 3, 4), RTE_IPV4(5, 6, 7, 8)} };
> > > > > >
> > > > > > @@ -150,8 +150,8 @@ static struct rte_flow_item tcp_item_1 =
> > > > > > { RTE_FLOW_ITEM_TYPE_TCP,
> > > > > > * dst mask 255.255.255.00 / sctp src is 16 dst is 17/ end"
> > > > > > */
> > > > > > static struct rte_flow_item_ipv4 ipv4_sctp_spec_1 = {
> > > > > > - { 0, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0, RTE_IPV4(11, 12, 13,
> > > > > > 14),
> > > > > > - RTE_IPV4(15, 16, 17, 18)}
> > > > > > + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0,
> > > > > > + RTE_IPV4(11, 12, 13, 14), RTE_IPV4(15, 16, 17, 18)}
> > > > > > };
> > > > > >
> > > > > > static struct rte_flow_item_sctp sctp_spec_1 = { diff --git
> > > > > > a/lib/net/rte_ip.h b/lib/net/rte_ip.h index
> > > > > > 4b728969c1..684bb028b2
> > > > > > 100644
> > > > > > --- a/lib/net/rte_ip.h
> > > > > > +++ b/lib/net/rte_ip.h
> > > > > > @@ -38,7 +38,21 @@ extern "C" {
> > > > > > * IPv4 Header
> > > > > > */
> > > > > > struct rte_ipv4_hdr {
> > > > > > - uint8_t version_ihl; /**< version and header length */
> > > > > > + __extension__
> > > > > > + union {
> > > > > > + uint8_t version_ihl; /**< version and header length */
> > > > > > + struct {
> > > > > > +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> > > > > > + uint8_t ihl:4;
> > > > > > + uint8_t version:4; #elif RTE_BYTE_ORDER ==
> > > > > > +RTE_BIG_ENDIAN
> > > > > > + uint8_t version:4;
> > > > > > + uint8_t ihl:4; #else #error "setup endian
> > > > > > +definition"
> > > > > > +#endif
> > > > > > + };
> > > > > > + };
> > > > > > uint8_t type_of_service; /**< type of service */
> > > > > > rte_be16_t total_length; /**< length of packet */
> > > > > > rte_be16_t packet_id; /**< packet ID */
> > > > > > --
> > > > > > 2.31.1
> > > > > >
> > > > >
> > > > > This does not break the ABI, but it could be discussed if it
> > > > > breaks
> > > > the API due to the required structure initialization changes shown
> > > > in
> > > > > test_flow_classify.c.
> > > >
> > > > Yep, I guess it might be classified as API change.
> > > > Another thing that concerns me - it is not the only place in IPv4
> > > > header when we unite multiple bit-fields into one field:
> > > > type_of_service, fragment_offset.
> > > > If we start splitting ipv4 fields into actual bitfields, I suppose
> > > > we'll end-up splitting these ones too.
> > > > But I am not sure it will pay off - as compiler not always
> > > > generates optimal code for reading/updating bitfields.
> > > > Did you consider just adding extra macros to simplify access to
> > > > these fields (like RTE_IPV4_HDR_(GET_SET)_*), instead?
> > > >
> > >
> > > Let's please not introduce accessor macros for bitfields. If we
> > > don't introduce bitfields like these, I would rather stick with the
> > > current _MASK, _SHIFT and _FLAG defines.
> > >
> > > Yes, this change will lead to the introduction of more bitfields,
> > > both here and in other places. We already accepted it in the eCPRI
> > > structure (/lib/net/rte_ecpri.h), so why not just generally accept it.
> > >
> > > Are modern compilers really worse at handling a bitfield defined
> > > like this, compared to handling a single uint8_t with hand coding? I
> > > consider your concern very important, so I'm only asking if it is
> > > still relevant, to avoid making decisions based on past experience
> > > that might be outdated. (I admit to falling into that trap myself,
> > > once in a while.)
> > >
> >
> > I compared x86 code generated with gcc-9, gcc-10 and clang-10 for these
> 2 functions:
> > void test_ipv4_hdr_byte(struct rte_ipv4_hdr *h, uint8_t version,
> > uint8_t ihl) {
> > h->version_ihl = ((version & 0x0f) << 4) | (ihl & 0x0f); } void
> > test_ipv4_hdr_bits(struct rte_ipv4_hdr *h, uint8_t version, uint8_t
> > ihl) {
> > h->version = version & 0x0f;
> > h->ihl = ihl & 0x0f;
> > }
> > meson configuration flags: --default-library=static
> > --buildtype=release Each compiler produced identical code for both
> functions.
>
> For that particular case (2 bit-fields packed tightly into one byte) compilers
> usually perform quite well. At least I never saw issues for such case.
> Bit-fields that do cross byte boundaries - that might be a trouble.
>
Can we keep both implementations, the combined byte and the bit-field,
grouped into a union ? In that case application or PMD can select access
method that fits.
> >
> >
> > > > > I think this patch is an improvement, and that such structure
> > > > modifications should be generally accepted, so:
> > > > >
> > > > > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > > >
Hello,
Is there another concern about that patch ?
Please comment.
Regards,
Gregory
> -----Original Message-----
> From: Gregory Etelson
> Sent: Monday, May 31, 2021 14:10
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Morten Brørup
> <mb@smartsharesystems.com>; dev@dpdk.org
> Cc: Matan Azrad <matan@nvidia.com>; Ori Kam <orika@nvidia.com>;
> Raslan Darawsheh <rasland@nvidia.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>; Olivier Matz <olivier.matz@6wind.com>
> Subject: RE: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields
>
> > > > > > > RTE IPv4 header definition combines the `version' and `ihl'
> > > > > > > fields into a single structure member.
> > > > > > > This patch introduces dedicated structure members for both
> > > > > `version'
> > > > > > > and `ihl' IPv4 fields. Separated header fields definitions
> > > > > > > allow to create simplified code to match on the IHL value in
> > > > > > > a flow
> > rule.
> > > > > > > The original `version_ihl' structure member is kept for
> > > > > > > backward compatibility.
> > > > > > >
> > > > > > > Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> > > > > > > ---
> > > > > > > app/test/test_flow_classify.c | 8 ++++----
> > > > > > > lib/net/rte_ip.h | 16 +++++++++++++++-
> > > > > > > 2 files changed, 19 insertions(+), 5 deletions(-)
> > > > > > >
> > > > > > > diff --git a/app/test/test_flow_classify.c
> > > > > > > b/app/test/test_flow_classify.c index 951606f248..4f64be5357
> > > > > > > 100644
> > > > > > > --- a/app/test/test_flow_classify.c
> > > > > > > +++ b/app/test/test_flow_classify.c
> > > > > > > @@ -95,7 +95,7 @@ static struct rte_acl_field_def
> > > > > > > ipv4_defs[NUM_FIELDS_IPV4] = {
> > > > > > > * dst mask 255.255.255.00 / udp src is 32 dst is 33 / end"
> > > > > > > */
> > > > > > > static struct rte_flow_item_ipv4 ipv4_udp_spec_1 = {
> > > > > > > - { 0, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
> > > > > > > + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
> > > > > > > RTE_IPV4(2, 2, 2, 3), RTE_IPV4(2, 2, 2, 7)} }; static
> > > > > > > const struct rte_flow_item_ipv4 ipv4_mask_24 = { @@ -131,7
> > > > > > > +131,7 @@ static struct rte_flow_item end_item = {
> > > > RTE_FLOW_ITEM_TYPE_END,
> > > > > > > * dst mask 255.255.255.00 / tcp src is 16 dst is 17 / end"
> > > > > > > */
> > > > > > > static struct rte_flow_item_ipv4 ipv4_tcp_spec_1 = {
> > > > > > > - { 0, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
> > > > > > > + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
> > > > > > > RTE_IPV4(1, 2, 3, 4), RTE_IPV4(5, 6, 7, 8)} };
> > > > > > >
> > > > > > > @@ -150,8 +150,8 @@ static struct rte_flow_item tcp_item_1
> > > > > > > = { RTE_FLOW_ITEM_TYPE_TCP,
> > > > > > > * dst mask 255.255.255.00 / sctp src is 16 dst is 17/ end"
> > > > > > > */
> > > > > > > static struct rte_flow_item_ipv4 ipv4_sctp_spec_1 = {
> > > > > > > - { 0, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0, RTE_IPV4(11, 12, 13,
> > > > > > > 14),
> > > > > > > - RTE_IPV4(15, 16, 17, 18)}
> > > > > > > + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0,
> > > > > > > + RTE_IPV4(11, 12, 13, 14), RTE_IPV4(15, 16, 17, 18)}
> > > > > > > };
> > > > > > >
> > > > > > > static struct rte_flow_item_sctp sctp_spec_1 = { diff --git
> > > > > > > a/lib/net/rte_ip.h b/lib/net/rte_ip.h index
> > > > > > > 4b728969c1..684bb028b2
> > > > > > > 100644
> > > > > > > --- a/lib/net/rte_ip.h
> > > > > > > +++ b/lib/net/rte_ip.h
> > > > > > > @@ -38,7 +38,21 @@ extern "C" {
> > > > > > > * IPv4 Header
> > > > > > > */
> > > > > > > struct rte_ipv4_hdr {
> > > > > > > - uint8_t version_ihl; /**< version and header length */
> > > > > > > + __extension__
> > > > > > > + union {
> > > > > > > + uint8_t version_ihl; /**< version and header length */
> > > > > > > + struct {
> > > > > > > +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> > > > > > > + uint8_t ihl:4;
> > > > > > > + uint8_t version:4; #elif RTE_BYTE_ORDER ==
> > > > > > > +RTE_BIG_ENDIAN
> > > > > > > + uint8_t version:4;
> > > > > > > + uint8_t ihl:4; #else #error "setup endian
> > > > > > > +definition"
> > > > > > > +#endif
> > > > > > > + };
> > > > > > > + };
> > > > > > > uint8_t type_of_service; /**< type of service */
> > > > > > > rte_be16_t total_length; /**< length of packet */
> > > > > > > rte_be16_t packet_id; /**< packet ID */
> > > > > > > --
> > > > > > > 2.31.1
> > > > > > >
> > > > > >
> > > > > > This does not break the ABI, but it could be discussed if it
> > > > > > breaks
> > > > > the API due to the required structure initialization changes
> > > > > shown in
> > > > > > test_flow_classify.c.
> > > > >
> > > > > Yep, I guess it might be classified as API change.
> > > > > Another thing that concerns me - it is not the only place in
> > > > > IPv4 header when we unite multiple bit-fields into one field:
> > > > > type_of_service, fragment_offset.
> > > > > If we start splitting ipv4 fields into actual bitfields, I
> > > > > suppose we'll end-up splitting these ones too.
> > > > > But I am not sure it will pay off - as compiler not always
> > > > > generates optimal code for reading/updating bitfields.
> > > > > Did you consider just adding extra macros to simplify access to
> > > > > these fields (like RTE_IPV4_HDR_(GET_SET)_*), instead?
> > > > >
> > > >
> > > > Let's please not introduce accessor macros for bitfields. If we
> > > > don't introduce bitfields like these, I would rather stick with
> > > > the current _MASK, _SHIFT and _FLAG defines.
> > > >
> > > > Yes, this change will lead to the introduction of more bitfields,
> > > > both here and in other places. We already accepted it in the eCPRI
> > > > structure (/lib/net/rte_ecpri.h), so why not just generally accept it.
> > > >
> > > > Are modern compilers really worse at handling a bitfield defined
> > > > like this, compared to handling a single uint8_t with hand coding?
> > > > I consider your concern very important, so I'm only asking if it
> > > > is still relevant, to avoid making decisions based on past
> > > > experience that might be outdated. (I admit to falling into that
> > > > trap myself, once in a while.)
> > > >
> > >
> > > I compared x86 code generated with gcc-9, gcc-10 and clang-10 for
> > > these
> > 2 functions:
> > > void test_ipv4_hdr_byte(struct rte_ipv4_hdr *h, uint8_t version,
> > > uint8_t ihl) {
> > > h->version_ihl = ((version & 0x0f) << 4) | (ihl & 0x0f); }
> > > void test_ipv4_hdr_bits(struct rte_ipv4_hdr *h, uint8_t version,
> > > uint8_t
> > > ihl) {
> > > h->version = version & 0x0f;
> > > h->ihl = ihl & 0x0f;
> > > }
> > > meson configuration flags: --default-library=static
> > > --buildtype=release Each compiler produced identical code for both
> > functions.
> >
> > For that particular case (2 bit-fields packed tightly into one byte)
> > compilers usually perform quite well. At least I never saw issues for such
> case.
> > Bit-fields that do cross byte boundaries - that might be a trouble.
> >
>
> Can we keep both implementations, the combined byte and the bit-field,
> grouped into a union ? In that case application or PMD can select access
> method that fits.
>
> > >
> > >
> > > > > > I think this patch is an improvement, and that such structure
> > > > > modifications should be generally accepted, so:
> > > > > >
> > > > > > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > > > >
Hi, Morten and all,
I have a questions which has bothering me for a long time.
What's the difference between API and ABI？
Why does this patch does not breake ABI， but break API(maybe)?
Hope for your reply, thanks.
在 2021/5/27 23:56, Morten Brørup 写道:
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Gregory Etelson
>> Sent: Thursday, 27 May 2021 17.29
> and version fields
>>
>> RTE IPv4 header definition combines the `version' and `ihl' fields
>> into a single structure member.
>> This patch introduces dedicated structure members for both `version'
>> and `ihl' IPv4 fields. Separated header fields definitions allow to
>> create simplified code to match on the IHL value in a flow rule.
>> The original `version_ihl' structure member is kept for backward
>> compatibility.
>>
>> Signed-off-by: Gregory Etelson <getelson@nvidia.com>
>> ---
>> app/test/test_flow_classify.c | 8 ++++----
>> lib/net/rte_ip.h | 16 +++++++++++++++-
>> 2 files changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/app/test/test_flow_classify.c
>> b/app/test/test_flow_classify.c
>> index 951606f248..4f64be5357 100644
>> --- a/app/test/test_flow_classify.c
>> +++ b/app/test/test_flow_classify.c
>> @@ -95,7 +95,7 @@ static struct rte_acl_field_def
>> ipv4_defs[NUM_FIELDS_IPV4] = {
>> * dst mask 255.255.255.00 / udp src is 32 dst is 33 / end"
>> */
>> static struct rte_flow_item_ipv4 ipv4_udp_spec_1 = {
>> - { 0, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
>> + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
>> RTE_IPV4(2, 2, 2, 3), RTE_IPV4(2, 2, 2, 7)}
>> };
>> static const struct rte_flow_item_ipv4 ipv4_mask_24 = {
>> @@ -131,7 +131,7 @@ static struct rte_flow_item end_item = {
>> RTE_FLOW_ITEM_TYPE_END,
>> * dst mask 255.255.255.00 / tcp src is 16 dst is 17 / end"
>> */
>> static struct rte_flow_item_ipv4 ipv4_tcp_spec_1 = {
>> - { 0, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
>> + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
>> RTE_IPV4(1, 2, 3, 4), RTE_IPV4(5, 6, 7, 8)}
>> };
>>
>> @@ -150,8 +150,8 @@ static struct rte_flow_item tcp_item_1 = {
>> RTE_FLOW_ITEM_TYPE_TCP,
>> * dst mask 255.255.255.00 / sctp src is 16 dst is 17/ end"
>> */
>> static struct rte_flow_item_ipv4 ipv4_sctp_spec_1 = {
>> - { 0, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0, RTE_IPV4(11, 12, 13, 14),
>> - RTE_IPV4(15, 16, 17, 18)}
>> + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0,
>> + RTE_IPV4(11, 12, 13, 14), RTE_IPV4(15, 16, 17, 18)}
>> };
>>
>> static struct rte_flow_item_sctp sctp_spec_1 = {
>> diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
>> index 4b728969c1..684bb028b2 100644
>> --- a/lib/net/rte_ip.h
>> +++ b/lib/net/rte_ip.h
>> @@ -38,7 +38,21 @@ extern "C" {
>> * IPv4 Header
>> */
>> struct rte_ipv4_hdr {
>> - uint8_t version_ihl; /**< version and header length */
>> + __extension__
>> + union {
>> + uint8_t version_ihl; /**< version and header length */
>> + struct {
>> +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
>> + uint8_t ihl:4;
>> + uint8_t version:4;
>> +#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
>> + uint8_t version:4;
>> + uint8_t ihl:4;
>> +#else
>> +#error "setup endian definition"
>> +#endif
>> + };
>> + };
>> uint8_t type_of_service; /**< type of service */
>> rte_be16_t total_length; /**< length of packet */
>> rte_be16_t packet_id; /**< packet ID */
>> --
>> 2.31.1
>>
>
> This does not break the ABI, but it could be discussed if it breaks the API due to the required structure initialization changes shown in test_flow_classify.c. I think this patch is an improvement, and that such structure modifications should be generally accepted, so:
>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
>
> .
>
On Thu, 3 Jun 2021 08:58:42 +0800
"Min Hu (Connor)" <humin29@huawei.com> wrote:
> Hi, Morten and all,
> I have a questions which has bothering me for a long time.
> What's the difference between API and ABI？
> Why does this patch does not breake ABI， but break API(maybe)?
>
> Hope for your reply, thanks.
The API being fixed, that a user can in confidence recompile their source
code and it will compile without any new errors.
The ABI guarantee means, that an application dynamically linked to DPDK
shared libraries will work without problem if the DPDK libraries are updated.
> On Thu, 3 Jun 2021 08:58:42 +0800
> "Min Hu (Connor)" <humin29@huawei.com> wrote:
>
> > Hi, Morten and all,
> > I have a questions which has bothering me for a long time.
> > What's the difference between API and ABI？
> > Why does this patch does not breake ABI， but break API(maybe)?
> >
> > Hope for your reply, thanks.
>
> The API being fixed, that a user can in confidence recompile their source
> code and it will compile without any new errors.
>
> The ABI guarantee means, that an application dynamically linked to DPDK
> shared libraries will work without problem if the DPDK libraries are
> updated.
Hello Stephen,
Thank you for the clarification.
According to the above statements, the patch introduces alternative
access method to IPv4 version & ihl fields without breaking existing API.
Regards,
Gregory
Hello,
There was no activity that patch for a long time.
The patch is marked as failed, but we verified failed tests and concluded that the failures can be ignored.
https://patchwork.dpdk.org/project/dpdk/patch/20210527152858.13312-1-getelson@nvidia.com/
How should I proceed with this case ?
Please advise.
Thank you.
Regards,
Gregory
> -----Original Message-----
> From: Gregory Etelson
> Sent: Wednesday, June 2, 2021 12:52
> To: Morten Brørup <mb@smartsharesystems.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>; dev@dpdk.org
> Cc: Matan Azrad <matan@nvidia.com>; Ori Kam <orika@nvidia.com>;
> Raslan Darawsheh <rasland@nvidia.com>; Olivier Matz
> <olivier.matz@6wind.com>; Thomas Monjalon <tmonjalon@nvidia.com>
> Subject: RE: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields
>
> Hello,
>
> Is there another concern about that patch ?
> Please comment.
>
> Regards,
> Gregory
>
> > -----Original Message-----
> > From: Gregory Etelson
> > Sent: Monday, May 31, 2021 14:10
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Morten
> Brørup
> > <mb@smartsharesystems.com>; dev@dpdk.org
> > Cc: Matan Azrad <matan@nvidia.com>; Ori Kam <orika@nvidia.com>;
> Raslan
> > Darawsheh <rasland@nvidia.com>; Iremonger, Bernard
> > <bernard.iremonger@intel.com>; Olivier Matz
> <olivier.matz@6wind.com>
> > Subject: RE: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version
> > fields
> >
> > > > > > > > RTE IPv4 header definition combines the `version' and `ihl'
> > > > > > > > fields into a single structure member.
> > > > > > > > This patch introduces dedicated structure members for both
> > > > > > `version'
> > > > > > > > and `ihl' IPv4 fields. Separated header fields definitions
> > > > > > > > allow to create simplified code to match on the IHL value
> > > > > > > > in a flow
> > > rule.
> > > > > > > > The original `version_ihl' structure member is kept for
> > > > > > > > backward compatibility.
> > > > > > > >
> > > > > > > > Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> > > > > > > > ---
> > > > > > > > app/test/test_flow_classify.c | 8 ++++----
> > > > > > > > lib/net/rte_ip.h | 16 +++++++++++++++-
> > > > > > > > 2 files changed, 19 insertions(+), 5 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/app/test/test_flow_classify.c
> > > > > > > > b/app/test/test_flow_classify.c index
> > > > > > > > 951606f248..4f64be5357
> > > > > > > > 100644
> > > > > > > > --- a/app/test/test_flow_classify.c
> > > > > > > > +++ b/app/test/test_flow_classify.c
> > > > > > > > @@ -95,7 +95,7 @@ static struct rte_acl_field_def
> > > > > > > > ipv4_defs[NUM_FIELDS_IPV4] = {
> > > > > > > > * dst mask 255.255.255.00 / udp src is 32 dst is 33 / end"
> > > > > > > > */
> > > > > > > > static struct rte_flow_item_ipv4 ipv4_udp_spec_1 = {
> > > > > > > > - { 0, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
> > > > > > > > + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
> > > > > > > > RTE_IPV4(2, 2, 2, 3), RTE_IPV4(2, 2, 2, 7)} };
> > > > > > > > static const struct rte_flow_item_ipv4 ipv4_mask_24 = { @@
> > > > > > > > -131,7
> > > > > > > > +131,7 @@ static struct rte_flow_item end_item = {
> > > > > RTE_FLOW_ITEM_TYPE_END,
> > > > > > > > * dst mask 255.255.255.00 / tcp src is 16 dst is 17 / end"
> > > > > > > > */
> > > > > > > > static struct rte_flow_item_ipv4 ipv4_tcp_spec_1 = {
> > > > > > > > - { 0, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
> > > > > > > > + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
> > > > > > > > RTE_IPV4(1, 2, 3, 4), RTE_IPV4(5, 6, 7, 8)} };
> > > > > > > >
> > > > > > > > @@ -150,8 +150,8 @@ static struct rte_flow_item
> > > > > > > > tcp_item_1 = { RTE_FLOW_ITEM_TYPE_TCP,
> > > > > > > > * dst mask 255.255.255.00 / sctp src is 16 dst is 17/ end"
> > > > > > > > */
> > > > > > > > static struct rte_flow_item_ipv4 ipv4_sctp_spec_1 = {
> > > > > > > > - { 0, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0, RTE_IPV4(11, 12,
> > > > > > > > 13, 14),
> > > > > > > > - RTE_IPV4(15, 16, 17, 18)}
> > > > > > > > + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0,
> > > > > > > > + RTE_IPV4(11, 12, 13, 14), RTE_IPV4(15, 16, 17, 18)}
> > > > > > > > };
> > > > > > > >
> > > > > > > > static struct rte_flow_item_sctp sctp_spec_1 = { diff
> > > > > > > > --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h index
> > > > > > > > 4b728969c1..684bb028b2
> > > > > > > > 100644
> > > > > > > > --- a/lib/net/rte_ip.h
> > > > > > > > +++ b/lib/net/rte_ip.h
> > > > > > > > @@ -38,7 +38,21 @@ extern "C" {
> > > > > > > > * IPv4 Header
> > > > > > > > */
> > > > > > > > struct rte_ipv4_hdr {
> > > > > > > > - uint8_t version_ihl; /**< version and header length */
> > > > > > > > + __extension__
> > > > > > > > + union {
> > > > > > > > + uint8_t version_ihl; /**< version and header length */
> > > > > > > > + struct {
> > > > > > > > +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> > > > > > > > + uint8_t ihl:4;
> > > > > > > > + uint8_t version:4; #elif RTE_BYTE_ORDER
> > > > > > > > +== RTE_BIG_ENDIAN
> > > > > > > > + uint8_t version:4;
> > > > > > > > + uint8_t ihl:4; #else #error "setup
> > > > > > > > +endian definition"
> > > > > > > > +#endif
> > > > > > > > + };
> > > > > > > > + };
> > > > > > > > uint8_t type_of_service; /**< type of service */
> > > > > > > > rte_be16_t total_length; /**< length of packet */
> > > > > > > > rte_be16_t packet_id; /**< packet ID */
> > > > > > > > --
> > > > > > > > 2.31.1
> > > > > > > >
> > > > > > >
> > > > > > > This does not break the ABI, but it could be discussed if it
> > > > > > > breaks
> > > > > > the API due to the required structure initialization changes
> > > > > > shown in
> > > > > > > test_flow_classify.c.
> > > > > >
> > > > > > Yep, I guess it might be classified as API change.
> > > > > > Another thing that concerns me - it is not the only place in
> > > > > > IPv4 header when we unite multiple bit-fields into one field:
> > > > > > type_of_service, fragment_offset.
> > > > > > If we start splitting ipv4 fields into actual bitfields, I
> > > > > > suppose we'll end-up splitting these ones too.
> > > > > > But I am not sure it will pay off - as compiler not always
> > > > > > generates optimal code for reading/updating bitfields.
> > > > > > Did you consider just adding extra macros to simplify access
> > > > > > to these fields (like RTE_IPV4_HDR_(GET_SET)_*), instead?
> > > > > >
> > > > >
> > > > > Let's please not introduce accessor macros for bitfields. If we
> > > > > don't introduce bitfields like these, I would rather stick with
> > > > > the current _MASK, _SHIFT and _FLAG defines.
> > > > >
> > > > > Yes, this change will lead to the introduction of more
> > > > > bitfields, both here and in other places. We already accepted it
> > > > > in the eCPRI structure (/lib/net/rte_ecpri.h), so why not just
> generally accept it.
> > > > >
> > > > > Are modern compilers really worse at handling a bitfield defined
> > > > > like this, compared to handling a single uint8_t with hand coding?
> > > > > I consider your concern very important, so I'm only asking if it
> > > > > is still relevant, to avoid making decisions based on past
> > > > > experience that might be outdated. (I admit to falling into that
> > > > > trap myself, once in a while.)
> > > > >
> > > >
> > > > I compared x86 code generated with gcc-9, gcc-10 and clang-10 for
> > > > these
> > > 2 functions:
> > > > void test_ipv4_hdr_byte(struct rte_ipv4_hdr *h, uint8_t version,
> > > > uint8_t ihl) {
> > > > h->version_ihl = ((version & 0x0f) << 4) | (ihl & 0x0f); }
> > > > void test_ipv4_hdr_bits(struct rte_ipv4_hdr *h, uint8_t version,
> > > > uint8_t
> > > > ihl) {
> > > > h->version = version & 0x0f;
> > > > h->ihl = ihl & 0x0f;
> > > > }
> > > > meson configuration flags: --default-library=static
> > > > --buildtype=release Each compiler produced identical code for both
> > > functions.
> > >
> > > For that particular case (2 bit-fields packed tightly into one byte)
> > > compilers usually perform quite well. At least I never saw issues
> > > for such
> > case.
> > > Bit-fields that do cross byte boundaries - that might be a trouble.
> > >
> >
> > Can we keep both implementations, the combined byte and the bit-field,
> > grouped into a union ? In that case application or PMD can select
> > access method that fits.
> >
> > > >
> > > >
> > > > > > > I think this patch is an improvement, and that such
> > > > > > > structure
> > > > > > modifications should be generally accepted, so:
> > > > > > >
> > > > > > > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > > > > >
Hi Gregory,
On Thu, Jun 10, 2021 at 04:10:25AM +0000, Gregory Etelson wrote:
> Hello,
>
> There was no activity that patch for a long time.
> The patch is marked as failed, but we verified failed tests and concluded that the failures can be ignored.
> https://patchwork.dpdk.org/project/dpdk/patch/20210527152858.13312-1-getelson@nvidia.com/
> How should I proceed with this case ?
> Please advise.
>
I like the idea of this patch: to me it is more convenient to access to
these fields with a bitfield. I don't see a problem about using
bitfields here, glibc or FreeBSD netinet/ip.h are doing the same.
However, as stated previously, this patch breaks the initialization API.
The DPDK ABI/API policy is described here:
http://doc.dpdk.org/guides/contributing/abi_policy.html#the-dpdk-abi-policy
From this document:
The API should only be changed for significant reasons, such as
performance enhancements. API breakages due to changes such as
reorganizing public structure fields for aesthetic or readability
purposes should be avoided.
So to follow the project policy, I think we should reject this path.
Regards,
Olivier
On 6/10/21 12:22 PM, Olivier Matz wrote:
> Hi Gregory,
>
> On Thu, Jun 10, 2021 at 04:10:25AM +0000, Gregory Etelson wrote:
>> Hello,
>>
>> There was no activity that patch for a long time.
>> The patch is marked as failed, but we verified failed tests and concluded that the failures can be ignored.
>> https://patchwork.dpdk.org/project/dpdk/patch/20210527152858.13312-1-getelson@nvidia.com/
>> How should I proceed with this case ?
>> Please advise.
>>
>
> I like the idea of this patch: to me it is more convenient to access to
> these fields with a bitfield. I don't see a problem about using
> bitfields here, glibc or FreeBSD netinet/ip.h are doing the same.
>
> However, as stated previously, this patch breaks the initialization API.
Very good point. I guess we overlooked it in a number of patches
with fix RTE flow API items to start from corresponding network
headers. We used unions there to avoid ABI breakage, but it looks
like we have broken initialization API anyway.
We should decide if initialization ABI breakage is a show-stopper
for RTE flow API items switching to use network protocol headers.
> The DPDK ABI/API policy is described here:
> http://doc.dpdk.org/guides/contributing/abi_policy.html#the-dpdk-abi-policy
>
>>From this document:
>
> The API should only be changed for significant reasons, such as
> performance enhancements. API breakages due to changes such as
> reorganizing public structure fields for aesthetic or readability
> purposes should be avoided.
>
> So to follow the project policy, I think we should reject this path.
>
> Regards,
> Olivier
>
On Thu, May 27, 2021 at 06:28:58PM +0300, Gregory Etelson wrote:
> diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
> index 4b728969c1..684bb028b2 100644
> --- a/lib/net/rte_ip.h
> +++ b/lib/net/rte_ip.h
> @@ -38,7 +38,21 @@ extern "C" {
> * IPv4 Header
> */
> struct rte_ipv4_hdr {
> - uint8_t version_ihl; /**< version and header length */
> + __extension__
this patch reduces compiler portability, though not strictly objecting
so long as the community accepts that it may lead to conditional
compilation having to be introduced in a future change.
please also be mindful of the impact of __attribute__ ((__packed__)) in
the presence of bitfields on gcc when evaluating abi compatibility.
> + union {
> + uint8_t version_ihl; /**< version and header length */
> + struct {
> +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> + uint8_t ihl:4;
> + uint8_t version:4;
> +#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> + uint8_t version:4;
> + uint8_t ihl:4;
> +#else
> +#error "setup endian definition"
> +#endif
> + };
> + };
> uint8_t type_of_service; /**< type of service */
> rte_be16_t total_length; /**< length of packet */
> rte_be16_t packet_id; /**< packet ID */
> --
> 2.31.1
On 6/14/2021 5:36 PM, Andrew Rybchenko wrote:
> On 6/10/21 12:22 PM, Olivier Matz wrote:
>> Hi Gregory,
>>
>> On Thu, Jun 10, 2021 at 04:10:25AM +0000, Gregory Etelson wrote:
>>> Hello,
>>>
>>> There was no activity that patch for a long time.
>>> The patch is marked as failed, but we verified failed tests and concluded
>>> that the failures can be ignored.
>>> https://patchwork.dpdk.org/project/dpdk/patch/20210527152858.13312-1-getelson@nvidia.com/
>>>
>>> How should I proceed with this case ?
>>> Please advise.
>>>
>>
>> I like the idea of this patch: to me it is more convenient to access to
>> these fields with a bitfield. I don't see a problem about using
>> bitfields here, glibc or FreeBSD netinet/ip.h are doing the same.
>>
>> However, as stated previously, this patch breaks the initialization API.
>
> Very good point. I guess we overlooked it in a number of patches
> with fix RTE flow API items to start from corresponding network
> headers. We used unions there to avoid ABI breakage, but it looks
> like we have broken initialization API anyway.
>
Hi Andrew,
What is broken with the flow API item updates, can you please give a sample?
> We should decide if initialization ABI breakage is a show-stopper
> for RTE flow API items switching to use network protocol headers.
>
>> The DPDK ABI/API policy is described here:
>> http://doc.dpdk.org/guides/contributing/abi_policy.html#the-dpdk-abi-policy
>>
>>> From this document:
>>
>> The API should only be changed for significant reasons, such as
>> performance enhancements. API breakages due to changes such as
>> reorganizing public structure fields for aesthetic or readability
>> purposes should be avoided.
>>
>> So to follow the project policy, I think we should reject this path.
>>
>> Regards,
>> Olivier
>>
>
@@ -95,7 +95,7 @@ static struct rte_acl_field_def ipv4_defs[NUM_FIELDS_IPV4] = {
* dst mask 255.255.255.00 / udp src is 32 dst is 33 / end"
*/
static struct rte_flow_item_ipv4 ipv4_udp_spec_1 = {
- { 0, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
+ { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
RTE_IPV4(2, 2, 2, 3), RTE_IPV4(2, 2, 2, 7)}
};
static const struct rte_flow_item_ipv4 ipv4_mask_24 = {
@@ -131,7 +131,7 @@ static struct rte_flow_item end_item = { RTE_FLOW_ITEM_TYPE_END,
* dst mask 255.255.255.00 / tcp src is 16 dst is 17 / end"
*/
static struct rte_flow_item_ipv4 ipv4_tcp_spec_1 = {
- { 0, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
+ { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
RTE_IPV4(1, 2, 3, 4), RTE_IPV4(5, 6, 7, 8)}
};
@@ -150,8 +150,8 @@ static struct rte_flow_item tcp_item_1 = { RTE_FLOW_ITEM_TYPE_TCP,
* dst mask 255.255.255.00 / sctp src is 16 dst is 17/ end"
*/
static struct rte_flow_item_ipv4 ipv4_sctp_spec_1 = {
- { 0, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0, RTE_IPV4(11, 12, 13, 14),
- RTE_IPV4(15, 16, 17, 18)}
+ { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0,
+ RTE_IPV4(11, 12, 13, 14), RTE_IPV4(15, 16, 17, 18)}
};
static struct rte_flow_item_sctp sctp_spec_1 = {
@@ -38,7 +38,21 @@ extern "C" {
* IPv4 Header
*/
struct rte_ipv4_hdr {
- uint8_t version_ihl; /**< version and header length */
+ __extension__
+ union {
+ uint8_t version_ihl; /**< version and header length */
+ struct {
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
+ uint8_t ihl:4;
+ uint8_t version:4;
+#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
+ uint8_t version:4;
+ uint8_t ihl:4;
+#else
+#error "setup endian definition"
+#endif
+ };
+ };
uint8_t type_of_service; /**< type of service */
rte_be16_t total_length; /**< length of packet */
rte_be16_t packet_id; /**< packet ID */