From: Jerin Jacob <jerinjacobk@gmail.com>
To: Thomas Monjalon <thomas@monjalon.net>
Cc: "Stephen Hemminger" <stephen@networkplumber.org>,
"Nithin Dabilpuram" <ndabilpuram@marvell.com>,
"Akhil Goyal" <gakhil@marvell.com>,
jerinj@marvell.com, dev@dpdk.org,
"Morten Brørup" <mb@smartsharesystems.com>,
techboard@dpdk.org
Subject: Re: [PATCH 1/3] security: introduce out of place support for inline ingress
Date: Fri, 19 May 2023 13:37:31 +0530 [thread overview]
Message-ID: <CALBAE1NLrHwfW4TXQw3J3yekrfCbp12mx5cjXtZ=ecHJvv_GtQ@mail.gmail.com> (raw)
In-Reply-To: <5925463.UjTJXf6HLC@thomas>
On Tue, Apr 25, 2023 at 4:11 AM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 18/04/2023 10:33, Jerin Jacob:
> > On Tue, Apr 11, 2023 at 11:36 PM Stephen Hemminger
> > <stephen@networkplumber.org> wrote:
> > >
> > > On Tue, 11 Apr 2023 15:34:07 +0530
> > > Nithin Dabilpuram <ndabilpuram@marvell.com> wrote:
> > >
> > > > diff --git a/lib/security/rte_security.h b/lib/security/rte_security.h
> > > > index 4bacf9fcd9..866cd4e8ee 100644
> > > > --- a/lib/security/rte_security.h
> > > > +++ b/lib/security/rte_security.h
> > > > @@ -275,6 +275,17 @@ struct rte_security_ipsec_sa_options {
> > > > */
> > > > uint32_t ip_reassembly_en : 1;
> > > >
> > > > + /** Enable out of place processing on inline inbound packets.
> > > > + *
> > > > + * * 1: Enable driver to perform Out-of-place(OOP) processing for this inline
> > > > + * inbound SA if supported by driver. PMD need to register mbuf
> > > > + * dynamic field using rte_security_oop_dynfield_register()
> > > > + * and security session creation would fail if dynfield is not
> > > > + * registered successfully.
> > > > + * * 0: Disable OOP processing for this session (default).
> > > > + */
> > > > + uint32_t ingress_oop : 1;
> > > > +
> > > > /** Reserved bit fields for future extension
> > > > *
> > > > * User should ensure reserved_opts is cleared as it may change in
> > > > @@ -282,7 +293,7 @@ struct rte_security_ipsec_sa_options {
> > > > *
> > > > * Note: Reduce number of bits in reserved_opts for every new option.
> > > > */
> > > > - uint32_t reserved_opts : 17;
> > > > + uint32_t reserved_opts : 16;
> > > > };
> > >
> > > NAK
> > > Let me repeat the reserved bit rant. YAGNI
> > >
> > > Reserved space is not usable without ABI breakage unless the existing
> > > code enforces that reserved space has to be zero.
> > >
> > > Just saying "User should ensure reserved_opts is cleared" is not enough.
> >
> > Yes. I think, we need to enforce to have _init functions for the
> > structures which is using reserved filed.
> >
> > On the same note on YAGNI, I am wondering why NOT introduce
> > RTE_NEXT_ABI marco kind of scheme to compile out ABI breaking changes.
> > By keeping RTE_NEXT_ABI disable by default, enable explicitly if user
> > wants it to avoid waiting for one year any ABI breaking changes.
> > There are a lot of "fixed appliance" customers (not OS distribution
> > driven customer) they are willing to recompile DPDK for new feature.
> > What we are loosing with this scheme?
>
> RTE_NEXT_ABI is described in the ABI policy.
> We are not doing it currently, but I think we could
> when it is not too much complicate in the code.
>
> The only problems I see are:
> - more #ifdef clutter
> - 2 binary versions to test
> - CI and checks must handle RTE_NEXT_ABI version
I think, we have two buckets of ABI breakages via RTE_NEXT_ABI
1) Changes that introduces compilation failures like adding new
argument to API or change API name etc
2) Structure size change which won't affect the compilation but breaks
the ABI for shared library usage.
I think, (1) is very distributive, and I don't see recently such
changes. I think, we should avoid (1) for non XX.11 releases.(or two
or three-year cycles if we decide that path)
The (2) comes are very common due to the fact HW features are
evolving. I think, to address the (2), we have two options
a) Have reserved fields and have _init() function to initialize the structures
b) Follow YAGNI style and introduce RTE_NEXT_ABI for structure size change.
The above concerns[1] can greatly reduce with option b OR option a.
[1]
1) more #ifdef clutter
For option (a) this is not needed or option (b) the clutter will be
limited, it will be around structure which add the new filed and
around the FULL block where new functions are added (not inside the
functions)
2) 2 binary versions to test
For option (a) this is not needed, for option (b) it is limited as for
new features only one needs to test another binary (rather than NOT
adding a new feature).
3) CI and checks must handle RTE_NEXT_ABI version
I think, it is cheap to add this, at least for compilation test.
IMO, We need to change the API break release to 3 year kind of time
frame to have very good end user experience
and allow ABI related change to get in every release and force
_rebuild_ shared objects in major LTS release.
I think, in this major LTS version(23.11) if we can decide (a) vs (b)
then we can align the code accordingly . e.s.p for (a) we need to add
_init() functions.
Thoughts?
next prev parent reply other threads:[~2023-05-19 8:08 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-09 8:56 [RFC 1/2] " Nithin Dabilpuram
2023-03-09 8:56 ` [RFC 2/2] test/security: add unittest for inline ingress oop Nithin Dabilpuram
2023-04-11 10:04 ` [PATCH 1/3] security: introduce out of place support for inline ingress Nithin Dabilpuram
2023-04-11 10:04 ` [PATCH 2/3] net/cnxk: support inline ingress out of place session Nithin Dabilpuram
2023-04-11 10:04 ` [PATCH 3/3] test/security: add unittest for inline ingress oop Nithin Dabilpuram
2023-04-11 18:05 ` [PATCH 1/3] security: introduce out of place support for inline ingress Stephen Hemminger
2023-04-18 8:33 ` Jerin Jacob
2023-04-24 22:41 ` Thomas Monjalon
2023-05-19 8:07 ` Jerin Jacob [this message]
2023-05-30 9:23 ` Jerin Jacob
2023-05-30 13:51 ` Thomas Monjalon
2023-05-31 9:26 ` Morten Brørup
2023-07-01 7:15 ` [PATCH] doc: announce addition of new security IPsec SA option Nithin Dabilpuram
2023-07-03 14:35 ` Akhil Goyal
2023-07-04 5:15 ` [PATCH v2] " Nithin Dabilpuram
2023-07-05 14:07 ` Jerin Jacob
2023-07-11 8:55 ` [EXT] " Akhil Goyal
2023-07-06 23:05 ` [PATCH] " Ji, Kai
2023-08-11 8:54 ` [PATCH 1/3] security: introduce out of place support for inline ingress Nithin Dabilpuram
2023-08-11 8:54 ` [PATCH 2/3] net/cnxk: support inline ingress out of place session Nithin Dabilpuram
2023-08-11 8:54 ` [PATCH 3/3] test/security: add unittest for inline ingress oop Nithin Dabilpuram
2023-09-19 19:55 ` [PATCH 1/3] security: introduce out of place support for inline ingress Akhil Goyal
2023-09-21 2:15 ` [PATCH v2 " Nithin Dabilpuram
2023-09-21 2:15 ` [PATCH v2 2/3] net/cnxk: support inline ingress out of place session Nithin Dabilpuram
2023-09-21 2:15 ` [PATCH v2 3/3] test/security: add unittest for inline ingress oop Nithin Dabilpuram
2023-09-21 10:44 ` [PATCH v2 1/3] security: introduce out of place support for inline ingress Akhil Goyal
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CALBAE1NLrHwfW4TXQw3J3yekrfCbp12mx5cjXtZ=ecHJvv_GtQ@mail.gmail.com' \
--to=jerinjacobk@gmail.com \
--cc=dev@dpdk.org \
--cc=gakhil@marvell.com \
--cc=jerinj@marvell.com \
--cc=mb@smartsharesystems.com \
--cc=ndabilpuram@marvell.com \
--cc=stephen@networkplumber.org \
--cc=techboard@dpdk.org \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).