Message-ID: <cfc622a9-4420-438b-a60a-5f7285689b9f@intel.com>
Date: Tue, 3 Sep 2024 15:24:53 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Vladimir Oltean <olteanv@...il.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 9/3/2024 3:13 PM, Vladimir Oltean wrote:
> 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?

Our documentation has both lsb, msb, and the width. I agree, and i think
it would benefit if the packing documentation suggests this as well.

> 
>>> 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.
>

Right.


>>> 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.
> 

Makes sense. I may need to do a little work on the ice driver to make
that fit well :D I think currently we have a packing for the Tx queue
context in different places.

>>> 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.
> 

Yea. Actually my entire initial motivation for starting looking at
reworking ice was the weird sizes used in our unpacked context
structure... :D

>> 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.

Ok sounds good. I'll take a look at this.

Powered by blists - more mailing lists