Message-ID: <20240903221358.eupqxac7chvzxp6e@skbuf>
Date: Wed, 4 Sep 2024 01:13:58 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Jacob Keller <jacob.e.keller@...el.com>
Cc: netdev <netdev@...r.kernel.org>,
	Anthony Nguyen <anthony.l.nguyen@...el.com>,
	Intel Wired LAN <intel-wired-lan@...ts.osuosl.org>,
	Przemek Kitszel <przemyslaw.kitszel@...el.com>
Subject: Re: [PATCH iwl-next v2 10/13] ice: use <linux/packing.h> for Tx and
 Rx queue context data

On Tue, Sep 03, 2024 at 02:16:59PM -0700, Jacob Keller wrote:
> I only kept my interface in terms of lsb+size because I did not want to
> attempt to re-write the table. I actually did re-write the table at
> first, and discovered a documentation bug because our documentation for
> the table has incorrect lsb/msb for some of the fields in some versions
> of the doc!
> 
> I ultimately don't mind switching to the packing convention of start/end
> (though my brain does have trouble sometimes thinking of the start as
> the higher bit...)

Well, you can keep the ICE_CTX_STORE() macro formatted in whichever way
you want, including Width+LSB, but reimplement it to generate a struct
packed_field, formatted as startbit+endbit (endbit=LSB, startbit=LSB+Width-1).
I actually encourage any user of the packing library to write the code
in the closest way possible to the field definitions as they appear in
the documentation. What do you think?

> > I came to the realization that this API would be nice exactly because
> > otherwise, you need to duplicate the field definitions, once for the
> > pack() call and once for the unpack(). But if they're tables, you don't.
> > 
> > I'm looking at ways in which this new API could solve all the shortcomings
> > I don't like with lib/packing in general. Without being even aware of
> > ICE_CTX_STORE, I had actually implemented the type conversion to smaller
> > unpacked u8/u16/u32 types in exactly the same way.
> 
> I think having this in the core API with a standardized table, along
> with support for unpacking the types would be great!

Cool then!

> > I also wish to do something about the way in which lib/packing deals
> > with errors. I don't think it's exactly great for every driver to have
> > to dump stack and ignore them. And also, since they're programming
> > errors, it's odd (and bad for performance) to perform the sanity checks
> > for every function call, instead of just once, say at boot time. So I
> > had thought of a third new API call: packed_fields_validate(), which
> > runs at module_init() in the consumer driver (ice, sja1105, ...) and
> > operates on the field tables.
> > 
> 
> It does seem reasonable to me that we can move the sanity checks here,
> especially since the main programmer error is if this table is incorrect
> in one of these ways.

Actually I did manage to cook something up which I like even more than
packed_fields_validate(): a compile-time sanity check. I'm not completely
happy with it, just reasonably happy. You'll see.

> > Basically it is a singular replacement for these existing verifications
> > in pack() and unpack():
> > 
> > 	/* startbit is expected to be larger than endbit, and both are
> > 	 * expected to be within the logically addressable range of the buffer.
> > 	 */
> > 	if (unlikely(startbit < endbit || startbit >= 8 * pbuflen))
> > 		/* Invalid function call */
> > 		return -EINVAL;
> > 
> > 	value_width = startbit - endbit + 1;
> > 	if (unlikely(value_width > 64))
> > 		return -ERANGE;
> > 
> > so you actually need to tell packed_fields_validate() what is the size
> > of the buffer you intend to run pack_fields() and unpack_fields() on.
> > I don't see it as a problem at all that the packed buffer size must be
> > fixed and known ahead of time - you also operate on fixed ICE_RXQ_CTX_SZ
> > and ... hmmm... txq_ctx[22]? sized buffers.
> > 
> 
> Yea, these are fixed sizes. Strictly I think we could have a macro
> defining the size of the Tx queue context as well....

Yeah, you'll need than when you'll see what I've prepared below :)

> > But this packed_fields_validate() idea quickly creates another set of 2
> > problems which I didn't get to the bottom of:
> > 
> > 1. Some sanity checks in pack() are dynamic and cannot be run at
> >    module_init() time. Like this:
> > 
> > 	/* Check if "uval" fits in "value_width" bits.
> > 	 * If value_width is 64, the check will fail, but any
> > 	 * 64-bit uval will surely fit.
> > 	 */
> > 	if (value_width < 64 && uval >= (1ull << value_width))
> > 		/* Cannot store "uval" inside "value_width" bits.
> > 		 * Truncating "uval" is most certainly not desirable,
> > 		 * so simply erroring out is appropriate.
> > 		 */
> > 		return -ERANGE;
> > 
> 
> If we support u8/u16/u32/u64 sizes as well, you could check that the
> size of the unpacked variable too. Could this data be in the table? Oh I
> guess technically not because we are checking if the actual value passed
> fits. I think keeping this but making it WARN would be sufficient?

The u8/u16/u32/u64 unpacked field size will absolutely be held in the
struct packed_field tables.

> >    The worse part is that the check is actually useful. It led to the
> >    quick identification of the bug behind commit 24deec6b9e4a ("net:
> >    dsa: sja1105: disallow C45 transactions on the BASE-TX MDIO bus"),
> >    rather than seeing a silent failure. But... I would still really like
> >    the revised lib/packing API to return void for pack_fields() and
> >    unpack_fields(). Not really sure how to reconcile these.
> > 
> 
> Since this is generally programmer error (not something where uAPI can
> affect it) what about making these WARN in the core?

Yes, I went for demoting the -ERANGE error in the truncation case to a
WARN() that is printed for both the "int pack()" as well as the new
"void __pack()" call. Practically, it doesn't make a big difference
whether we bail out or do nothing, as long as we loudly complain in
either case. It's the complaint that leads to the bug getting easily
identified and fixed.

> > 2. How to make sure that the pbuflen passed to packed_fields_validate()
> >    will be the same one as the pbuflen passed to all subsequent pack_fields()
> >    calls validated against that very pbuflen?
> 
> I guess you either duplicate the check and WARN, or you don't, and let
> it panic on the invalid memory? But I guess that would only actually
> panic with something like KASAN

Yeah, I'm still not too sure here. The length of the packed buffer still
needs to be "obvious to the eye", since the same length must be manually
passed to the sanity check as well as to the actual pack_fields() call,
otherwise "bad things" will happen. I believe in the users' ability to
structure their code in such a way that this is not hard. Especially
since the sanity checks now rely on BUILD_BUG_ON(), and that can be
technically be placed anywhere in the code, I expect the best place to
put it is exactly near the pack_fields() call. That way, it's the most
obvious that the buffer length declared for the sanity check is
identical to the one during actual usage. Especially if the usage can be
restricted to just one function or two.

> > Sorry for not posting code and just telling a story about it. There
> > isn't much to show other than unfinished ideas with conflicting
> > requirements. So I thought maybe I could use a little help with some
> > brainstorming. Of course, do let me know if you are not that interested
> > in making the ICE_CTX_STORE tables idea a part of the packing core.
> > 
> > Thanks!
> 
> I think moving this into core would be fantastic. Since pretty much
> every driver handles these sanity checks the same way, I also think that
> moving those into the core and making them WARN or similar seems
> reasonable, so we can make the pack/unpack as void.
> 
> It would be interesting to see a comparison of the resulting module size.

Yeah, I'm also very much interested in comparisons: text size vs rodata
size vs dynamic memory usage. With the pack_fields() API in the sja1105
driver, I can shrink an enormous amount of u64 unpacked structure fields
to just u8. I also really like the idea of compile-time sanity checks,
and I'm curious how much that matters in a benchmark or something.
Anyway, I don't have a complete rework at the moment, so there's that.

> How much of this do you have code for now?

Well, I do have code, but it's not yet complete :)
I've updated by https://github.com/vladimiroltean/linux/tree/packing-selftests
branch on top of your patch set. Until HEAD~1, I've tested the sja1105
driver and it still works. The last patch - the bulk of the conversion
actually - is extremely tedious and is not yet ready (it doesn't even
compile). Yet, with a bit of imagination, it should be enough to provide
an example and hopefully move the discussion forward.

Powered by blists - more mailing lists