Re: [Intel-wired-lan] [PATCH iwl-next 5/6] ice: Optimize switch recipe creation

[Simon Horman <horms@kernel.org>]

On Fri, Jun 28, 2024 at 03:39:27PM +0200, Marcin Szycik wrote:
> 
> 
> On 28.06.2024 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@linux.intel.com>
> >> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> >> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> >> Signed-off-by: Marcin Szycik <marcin.szycik@linux.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@kernel.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.
> 
> Yeah... it is a bit of a revolution, and unfortunately I don't think much of
> if could be separated into other patches. Maybe functions like
> fill_recipe_template() and bookkeep_recipe() would be good candidates.
> If there will be another version, I'll try to separate some of it.

Understood. TBH, I couldn't think of a great way to split it either.