Re: [PATCH 1/3] security: introduce out of place support for inline ingress

[Thomas Monjalon <thomas@monjalon.net>]

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