Re: [PATCH net-next v2 1/9] net: microchip: vcap: fix rule move for rules of coprime size

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: microchip: vcap: fix rule move for rules of coprime size

This reworks the VCAP rule move logic so that insert and delete compute a
single uniform address offset, aligned to the LCM of the rule sizes in the
moved block, instead of per-rule "optimal" offsets. The goal is to keep the
software rule addresses in sync with what the hardware move command does,
which becomes necessary once rules of coprime sizes (2 and 3) appear with
the LPM VCAP.

One question about the delete algorithm description in the changelog.

> On delete:
>
> 1. Initialize HW memory at the deleted rule's addresses before
>    moving, since the moved block may not fully cover them due to
>    padding.
> 2. Reclaim any padding between rules in the block.

[Severity: Low]
Is step 2 accurate for the new implementation?  As far as I can tell the
padding that sits between rules inside the moved block is preserved, not
reclaimed, because a single offset is applied to every rule in the block:

drivers/net/ethernet/microchip/vcap/vcap_api.c:vcap_move_rules_sw() {
	list_for_each_entry_continue(pos, &admin->rules, list)
		pos->addr -= move->offset;
}

Since every rule in the block is shifted by the same move->offset, the gaps
between rules within the block are structurally invariant across the move.

What appears to be reclaimed is the deleted rule's own space plus the
padding adjacent to its position, and only down to block_lcm granularity.
In the delete path unaligned_offset is always negative (block_first->addr +
block_first->size <= ri->addr <= base_addr), so the truncating branch is
taken in vcap_move_from_block():

drivers/net/ethernet/microchip/vcap/vcap_api.c:vcap_move_from_block() {
	unaligned_offset = (block_first->addr + block_first->size) - base_addr;
	if (unaligned_offset > 0)
		move->offset =
			((unaligned_offset + (block_lcm - 1)) / block_lcm) *
			block_lcm;
	else
		move->offset = (unaligned_offset / block_lcm) * block_lcm;
}

Integer division truncating toward zero leaves up to block_lcm - 1 addresses
of the gap unreclaimed at the boundary.

Could the changelog wording be adjusted to describe what is actually
reclaimed, for example the deleted rule's addresses plus adjacent padding,
rounded down to the block LCM?

> diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.c b/drivers/net/ethernet/microchip/vcap/vcap_api.c
> index 788c0728d7636..75c2ca54fddd2 100644
> --- a/drivers/net/ethernet/microchip/vcap/vcap_api.c
> +++ b/drivers/net/ethernet/microchip/vcap/vcap_api.c

[ ... ]

> @@ -2106,12 +2108,63 @@ static u32 vcap_set_rule_id(struct vcap_rule_internal *ri)
>  	return ri->data.id;
>  }
>  
> +static void vcap_move_from_block(u32 base_addr,
> +				 struct vcap_rule_internal *block_first,
> +				 struct vcap_rule_internal *block_last,
> +				 int block_lcm, struct vcap_rule_move *move)
> +{
> +	int unaligned_offset;
> +
> +	if (!block_first || !block_last)
> +		return;
> +
> +	move->addr = block_last->addr;
> +	move->count = block_first->addr + block_first->size - block_last->addr;
> +	/* Integer division rounds toward zero. We want to round toward +inf
> +	 * for the positive case (insertion) to ensure enough room.
> +	 */
> +	unaligned_offset = (block_first->addr + block_first->size) - base_addr;
> +	if (unaligned_offset > 0)
> +		move->offset =
> +			((unaligned_offset + (block_lcm - 1)) / block_lcm) *
> +			block_lcm;
> +	else
> +		move->offset = (unaligned_offset / block_lcm) * block_lcm;
> +}
> +
> +static void vcap_move_rules_sw(struct vcap_admin *admin,
> +			       struct vcap_rule_internal *pos,
> +			       struct vcap_rule_move const *move)
> +{
> +	if (move->count == 0 || move->offset == 0)
> +		return;
> +
> +	list_for_each_entry_continue(pos, &admin->rules, list)
> +		pos->addr -= move->offset;
> +}

[ ... ]
-- 
pw-bot: cr