Message-ID: <00d23c42-2611-45d7-9d25-5ad394477238@intel.com>
Date: Tue, 3 Sep 2024 14:16:59 -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/2/2024 5:08 PM, Vladimir Oltean wrote:
> On Wed, Aug 28, 2024 at 01:57:26PM -0700, Jacob Keller wrote:
>> The major difference with <linux/packing.h> is that it expects the unpacked
>> data will always be a u64. This is somewhat limiting, but can be worked
>> around by using a local temporary u64.
>>
>> As with the other implementations, we handle the error codes from pack()
>> with a pr_err and a call to dump_stack. These are unexpected as they should
>> only happen due to programmer error.
>>
>> Note that I initially tried implementing this as functions which just
>> repeatably called the ice_ctx_pack() function instead of using the
>> ice_rlan_ctx_info and ice_tlan_ctx_info arrays. This does work, but it has
>> a couple of downsides:
>>
>>  1) it wastes a significant amount of bytes in the text section, vs the
>>     savings from removing the RO data of the arrays.
>>
>>  2) this cost is made worse after implementing an unpack function, as we
>>     must duplicate the list of packings for the unpack function.
> 
> I agree with your concerns and with your decision of keeping the
> ICE_CTX_STORE tables. Actually I have some more unposted lib/packing
> changes which operate on struct packed_field arrays, very, very similar
> to the ICE_CTX_STORE tables. Essentially two new calls: pack_fields()
> and unpack_fields(), which perform the iteration inside the core library.
> (the only real difference being that I went for startbit and endbit in
> their definitions, rather than LSB+size).
> 

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

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

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

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

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

> 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

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

How much of this do you have code for now?

Powered by blists - more mailing lists