Message-ID: <20240903000800.ue77eim4664dhy4p@skbuf>
Date: Tue, 3 Sep 2024 03:08:00 +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 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 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 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.

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.

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;

   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.

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?

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!

Powered by blists - more mailing lists