Date: Fri, 28 Jun 2024 15:56:55 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Simon Horman <horms@...nel.org>, Marcin Szycik
	<marcin.szycik@...ux.intel.com>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
	<michal.swiatkowski@...ux.intel.com>
Subject: Re: [PATCH iwl-next 5/6] ice: Optimize switch recipe creation

On 6/28/24 14:44, Simon Horman wrote:
> On Tue, Jun 18, 2024 at 04:11:56PM +0200, Marcin Szycik wrote:
>> Currently when creating switch recipes, switch ID is always added as the
>> first word in every recipe. There are only 5 words in a recipe, so one
>> word is always wasted. This is also true for the last recipe, which stores
>> result indexes (in case of chain recipes). Therefore the maximum usable
>> length of a chain recipe is 4 * 4 = 16 words. 4 words in a recipe, 4
>> recipes that can be chained (using a 5th one for result indexes).
>>
>> Current max size chained recipe:
>> 0: smmmm
>> 1: smmmm
>> 2: smmmm
>> 3: smmmm
>> 4: srrrr
>>
>> Where:
>> s - switch ID
>> m - regular match (e.g. ipv4 src addr, udp dst port, etc.)
>> r - result index
>>
>> Switch ID does not actually need to be present in every recipe, only in one
>> of them (in case of chained recipe). This frees up to 8 extra words:
>> 3 from recipes in the middle (because first recipe still needs to have
>> switch ID), and 5 from one extra recipe (because now the last recipe also
>> does not have switch ID, so it can chain 1 more recipe).
>>
>> Max size chained recipe after changes:
>> 0: smmmm
>> 1: Mmmmm
>> 2: Mmmmm
>> 3: Mmmmm
>> 4: MMMMM
>> 5: Rrrrr
>>
>> Extra usable words available after this change are highlighted with capital
>> letters.
>>
>> Changing how switch ID is added is not straightforward, because it's not a
>> regular lookup. Its FV index and mask can't be determined based on protocol
>> + offset pair read from package and instead need to be added manually.
>>
>> Additionally, change how result indexes are added. Currently they are
>> always inserted in a new recipe at the end. Example for 13 words, (with
>> above optimization, switch ID being one of the words):
>> 0: smmmm
>> 1: mmmmm
>> 2: mmmxx
>> 3: rrrxx
>>
>> Where:
>> x - unused word
>>
>> In this and some other cases, the result indexes can be moved just after
>> last matches because there are unused words, saving one recipe. Example
>> for 13 words after both optimizations:
>> 0: smmmm
>> 1: mmmmm
>> 2: mmmrr
>>
>> Note how one less result index is needed in this case, because the last
>> recipe does not need to "link" to itself.
>>
>> There are cases when adding an additional recipe for result indexes cannot
>> be avoided. In that cases result indexes are all put in the last recipe.
>> Example for 14 words after both optimizations:
>> 0: smmmm
>> 1: mmmmm
>> 2: mmmmx
>> 3: rrrxx
>>
>> With these two changes, recipes/rules are more space efficient, allowing
>> more to be created in total.
>>
>> Co-developed-by: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
>> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
>> Signed-off-by: Marcin Szycik <marcin.szycik@...ux.intel.com>
> 
> I appreciate the detailed description above, it is very helpful.
> After a number of readings of this patch - it is complex -
> I was unable to find anything wrong. And I do like both the simplification
> and better hw utilisation that this patch (set) brings.
> 
> So from that perspective:
> 
> Reviewed-by: Simon Horman <horms@...nel.org>
> 
> I would say, however, that it might have been easier to review
> if somehow this patch was broken up into smaller pieces.
> I appreciate that, in a sense, that is what the other patches
> of this series do. But nonetheless... it is complex.
> 
> ...

all of the "bugs" that I have internally found for this patch were
addressed by commit msg or comment changes ;)
what about you reviewing also patch 7 from v3 of this series?

Powered by blists - more mailing lists