Re: [PATCH net-next 3/8] lib: packing: add pack_fields() and unpack_fields()

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Tue, Oct 22, 2024 at 12:11:36PM -0700, Jacob Keller wrote:
> On 10/19/2024 5:20 AM, Vladimir Oltean wrote:
> > On Fri, Oct 18, 2024 at 02:50:52PM -0700, Jacob Keller wrote:
> >> Przemek, Vladimir,
> >>
> >> What are your thoughts on the next steps here. Do we need to go back to
> >> the drawing board for how to handle these static checks?
> >>
> >> Do we try to reduce the size somewhat, or try to come up with a
> >> completely different approach to handling this? Do we revert back to
> >> run-time checks? Investigate some alternative for static checking that
> >> doesn't have this limitation requiring thousands of lines of macro?
> >>
> >> I'd like to figure out what to do next.
> > 
> > Please see the attached patch for an idea on how to reduce the size
> > of <include/generated/packing-checks.h>, in a way that should be
> > satisfactory for both ice and sja1105, as well as future users.
> 
> This trades off generating the macros for an increase in the config
> complexity. I suppose that is slightly better than generating thousands
> of lines of macro... The unused macros sit on disk in the include file,
> but i don't think they would impact the deployed code...

Sorry, conflicting requirements. There will be a trade-off somewhere between
performance (having sanity checks at compile time rather than run time),
size (offer a library-level mechanism for consumer drivers to perform their
compile-time sanity checks), complexity (only generate those sanity
checks which are requested by drivers) and flexibility (support whichever
order the consumer driver desires for the arrays of packed fields).
I believe performance should not be the one which has to suffer, because
packet processing is one of the potential use cases, and I wouldn't want
to lose that through design choices. The rest.. I'm more flexible on,
but still, they have to be satisfiable in a way that I can see.

> I'm still wondering if there is a different approach we can take to
> validate these structures.

I just want to say that I don't have any alternative proposals, nor will I
explore your sparse suggestion. I don't know enough about sparse to judge
whether something as 'custom' as the packing API is in scope for its
check_call_instruction() infrastructure, how well will that solution
deal with internal kernel API changes down the line, and I don't have
the time to learn enough to prototype something to find the maintainers'
answer to these questions, either. I strongly prefer to have the static
checks inside the kernel, together with the packing() API itself, so it
can be more easily altered.

Obviously you're still free to wait for more opinions and suggestions,
or to experiment with the sparse idea yourself.

Honestly, my opinion is that if we can avoid messing too much with the
top-level Kbuild file, this pretty much enters "no one really cares"
territory, as long as the code is generated only for the pack_fields()
users. This is, in fact, one of the reasons why the patch I attached
earlier compiles and runs the code-gen only when PACKING_CHECK_FIELDS
is defined.