[dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields

Gregory Etelson getelson at nvidia.com
Thu Jun 10 06:10:25 CEST 2021
Previous message (by thread): [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields
Next message (by thread): [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
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 at smartsharesystems.com>; Iremonger, Bernard
> <bernard.iremonger at intel.com>; dev at dpdk.org
> Cc: Matan Azrad <matan at nvidia.com>; Ori Kam <orika at nvidia.com>;
> Raslan Darawsheh <rasland at nvidia.com>; Olivier Matz
> <olivier.matz at 6wind.com>; Thomas Monjalon <tmonjalon at 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 at intel.com>; Morten
> Brørup
> > <mb at smartsharesystems.com>; dev at dpdk.org
> > Cc: Matan Azrad <matan at nvidia.com>; Ori Kam <orika at nvidia.com>;
> Raslan
> > Darawsheh <rasland at nvidia.com>; Iremonger, Bernard
> > <bernard.iremonger at intel.com>; Olivier Matz
> <olivier.matz at 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 at 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 at smartsharesystems.com>
> > > > > >
Previous message (by thread): [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields
Next message (by thread): [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
More information about the dev mailing list