On 1/12/26 4:53 PM, Numan Siddique wrote:
> On Mon, Jan 12, 2026 at 8:28 AM Eelco Chaudron <[email protected]> wrote:
>>
>>
>>
>> On 12 Jan 2026, at 14:26, Eelco Chaudron wrote:
>>
>>> On 20 Dec 2025, at 0:50, [email protected] wrote:
>>>
>>>> From: Numan Siddique <[email protected]>
>>
>> Forgot to mention the subject needs a component:
>>
>> ofproto-dpif: Limit the recirc_id to max of 2^28.
>>
>>>> netdev-offload-tc when offloading a flow to tc, uses the flow's
>>>> recirc_id as flower chain id. recirc_id is of type 'uint32_t'.
>>>> Kernel tc reserves the upper nibble (4 bits) of the tc flower's
>>>> chain index for extended action opcode [1]. If a flow's recirc_id
>>>> value is greater than 268,435,455 (2^28), kernel returns
>>>
>>> (2^28 - 1)
>
> Ack.
>
>
>>>
>>>> EINVAL when ovs-vswitchd tries to offload the flow.
>>>>
>>>> This commit fixes this offload issue by setting the maximum
>>>> value of recirc id to 2^28. 2^28 is a fairly big number and
>>>
>>> value of recirc id to 2^28 - 1. 2^28 - 1 is a fairly big number and
>>>
>>>> we should not have that many active datapath flows at a time.
>>>>
>>>> Reported-at:
>>>> https://mail.openvswitch.org/pipermail/ovs-dev/2025-November/427485.html
>>>> Suggested-by: Ilya Maximets <[email protected]>
>>>> Suggested-by: Eelco Chaudron <[email protected]>
>>>> Signed-off-by: Numan Siddique <[email protected]>
>>>> ---
>>>> ofproto/ofproto-dpif-rid.c | 13 ++++++++++---
>>>> 1 file changed, 10 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
>>>> index f01468025..09f8a84f1 100644
>>>> --- a/ofproto/ofproto-dpif-rid.c
>>>> +++ b/ofproto/ofproto-dpif-rid.c
>>>> @@ -38,6 +38,13 @@ static uint32_t next_id OVS_GUARDED_BY(mutex) = 1; /*
>>>> Possible next free id. */
>>>>
>>>> #define RECIRC_POOL_STATIC_IDS 1024
>>>>
>>>> +/* Limit the recirc_ic maximum value to 268435455 (which is 2^^28).
>>>
>>> recirc_ic -> recirc_id
>>>
>>> Guess this is not correct, it should be 2^^28 - 1 (same below).
>>>
>>>> + * When kernel tc offload is enabled, we use the recirc_id as
>>>> + * the chain id and kernel limits the chain id maximum value
>>>> + * to 2^^28.
>>>> + */
>>>> +#define RECIRC_ID_MAX_VALUE 268435455
>>>
>>> Would it make sense to use TC_ACT_EXT_VAL_MASK here? Not 100% sure about
>>> this, Ilya?
>
> We could. I thought about it. I was not sure if its ok to include
> "include/uapi/linux/pkt_cls.h" (or appropriate linux header file)
> in ofproto/ofprot-dpif-rid.c. Is that ok ?
Inclusion of platform-specific headers in ofproto is not good.
Should avoid that, if possible.
If we really want to have a compile-time proof that we're not out of range,
then we should define the value in some common header and add a build assert
to the linux-specific code. But I'm not sure this is necessary, as kernel
uAPI is unlikely to change. May mention the TC_ACT_EXT_VAL_MASK in the
comment though, e.g. "kernel limits the chain id maximum value to
TC_ACT_EXT_VAL_MASK (2^28 - 1)".
BTW, I'd suggest to define the value as ((UINT32_C(1) << 28) - 1) instead of
a "magic" number. This way you'll also not need to explain what it equals to
in the comments.
>
>
>
>>>
>>>> +
>>>> static void recirc_id_node_free(struct recirc_id_node *);
>>>>
>>>> /* This should be called by the revalidator once at each round (every
>>>> 500ms or
>>>> @@ -227,8 +234,8 @@ frozen_state_free(struct frozen_state *state)
>>>> }
>>>>
>>>> /* Allocate a unique recirculation id for the given set of flow metadata.
>>>> - * The ID space is 2^^32, so there should never be a situation in which
>>>> all
>>>> - * the IDs are used up. We loop until we find a free one. */
>>>> + * The ID space is limited to 2^^28, so there should never be a situation
>>>
>>> 2^^28 - 1, should we maybe also move to use a single carat for power of?
>>> Other comments in the code use a single caret.
>
> I used double carat because the existing comment used double carat (i.e
> 2^^32).
> I can change it to single. Please let me know.
>
It should be either 2^28 or 2**28. Current code uses ^^ and it is strange. :)
I'd suggest we don't follow this pattern.
Best regards, Ilya Maximets.
> Thanks
> Numan
>
>
>>>
>>>> + * in which all the IDs are used up. We loop until we find a free one. */
>>>> static struct recirc_id_node *
>>>> recirc_alloc_id__(const struct frozen_state *state, uint32_t hash)
>>>> {
>>>> @@ -247,7 +254,7 @@ recirc_alloc_id__(const struct frozen_state *state,
>>>> uint32_t hash)
>>>> RECIRC_POOL_STATIC_IDS IDs on the later rounds, though, as
>>>> some of
>>>> the initial allocations may be for long term uses (like
>>>> bonds). */
>>>> node->id = next_id++;
>>>> - if (OVS_UNLIKELY(!node->id)) {
>>>> + if (OVS_UNLIKELY(!node->id || node->id > RECIRC_ID_MAX_VALUE)) {
>>>> next_id = RECIRC_POOL_STATIC_IDS + 1;
>>>> node->id = next_id++;
>>>> }
>>>> --
>>>> 2.52.0
>>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
