On 2 Nov 2022, at 11:58, Roi Dayan wrote:
> On 31/10/2022 12:39, Eelco Chaudron wrote:
>>
>>
>> On 27 Oct 2022, at 13:58, Roi Dayan wrote:
>>
>>> From: Paul Blakey <[email protected]>
>>>
>>> Currently ethertype to prio hmap is static and the first ethertype
>>> being used gets a lower priority. Usually there is an arp request
>>> before the ip traffic and the arp ethertype gets a lower tc priority
>>> while the ip traffic proto gets a higher priority.
>>> In this case ip traffic will go through more hops in tc and HW.
>>> Instead, reserve lower priorities for ip ethertypes.
>>
>> Changes look good to me, only one little nit, but this is just personal
>> preference.
>> I did not do any specific testing other than the make check tests.
>>
>> Acked-by: Eelco Chaudron <[email protected]>
>>
>>
>>> Signed-off-by: Paul Blakey <[email protected]>
>>> Reviewed-by: Roi Dayan <[email protected]>
>>> ---
>>>
>>> Notes:
>>> v2:
>>> - don't reverse ip prio if multi mask per prio is not supported.
>>>
>>> lib/netdev-offload-tc.c | 33 +++++++++++++++++++++++++++------
>>> lib/tc.h | 2 ++
>>> 2 files changed, 29 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
>>> index f6f90a741fde..b392bbf441f4 100644
>>> --- a/lib/netdev-offload-tc.c
>>> +++ b/lib/netdev-offload-tc.c
>>> @@ -325,6 +325,28 @@ struct prio_map_data {
>>> uint16_t prio;
>>> };
>>>
>>> +static uint16_t
>>> +get_next_available_prio(ovs_be16 protocol)
>>> +{
>>> + static uint16_t last_prio = TC_RESERVED_PRIORITY_MAX;
>>> +
>>> + if (multi_mask_per_prio) {
>>> + if (protocol == htons(ETH_P_IP)) {
>>> + return TC_RESERVED_PRIORITY_IPV4;
>>> + } else if (protocol == htons(ETH_P_IPV6)) {
>>> + return TC_RESERVED_PRIORITY_IPV6;
>>> + }
>>> + }
>>> +
>>> + /* last_prio can overflow if there will be many different kinds of
>>> + * flows which shouldn't happen organically. */
>>> + if (last_prio == UINT16_MAX) {
>>> + return 0;
>>
>> nit: return TC_RESERVED_PRIORITY_NONE;
>
> ok.
>
>>
>>> + }
>>> +
>>> + return ++last_prio;
>>> +}
>>> +
>>> /* Get free prio for tc flower
>>> * If prio is already allocated for mask/eth_type combination then return
>>> it.
>>> * If not assign new prio.
>>> @@ -336,11 +358,11 @@ get_prio_for_tc_flower(struct tc_flower *flower)
>>> {
>>> static struct hmap prios = HMAP_INITIALIZER(&prios);
>>> static struct ovs_mutex prios_lock = OVS_MUTEX_INITIALIZER;
>>> - static uint16_t last_prio = TC_RESERVED_PRIORITY_MAX;
>>> size_t key_len = sizeof(struct tc_flower_key);
>>> size_t hash = hash_int((OVS_FORCE uint32_t) flower->key.eth_type, 0);
>>> struct prio_map_data *data;
>>> struct prio_map_data *new_data;
>>> + uint16_t prio;
>>>
>>> if (!multi_mask_per_prio) {
>>> hash = hash_bytes(&flower->mask, key_len, hash);
>>> @@ -359,21 +381,20 @@ get_prio_for_tc_flower(struct tc_flower *flower)
>>> }
>>> }
>>>
>>> - if (last_prio == UINT16_MAX) {
>>> - /* last_prio can overflow if there will be many different kinds of
>>> - * flows which shouldn't happen organically. */
>>> + prio = get_next_available_prio(flower->key.eth_type);
>>> + if (!prio) {
>>
>> nit: if (prio == TC_RESERVED_PRIORITY_NONE) {
>
> ok
>
>>
>>> ovs_mutex_unlock(&prios_lock);
>>> return 0;
>>
>> nit: return TC_RESERVED_PRIORITY_NONE
>
> how about just return prio ? as the comparison is
> against TC_RESERVED_PRIORITY_NONE only.
Both will work, I have no preference.
>>
>>> }
>>>
>>> new_data = xzalloc(sizeof *new_data);
>>> memcpy(&new_data->mask, &flower->mask, key_len);
>>> - new_data->prio = ++last_prio;
>>> + new_data->prio = prio;
>>> new_data->protocol = flower->key.eth_type;
>>> hmap_insert(&prios, &new_data->node, hash);
>>> ovs_mutex_unlock(&prios_lock);
>>>
>>> - return new_data->prio;
>>> + return prio;
>>> }
>>>
>>> static uint32_t
>>> diff --git a/lib/tc.h b/lib/tc.h
>>> index 2e64ad372592..12753c16d405 100644
>>> --- a/lib/tc.h
>>> +++ b/lib/tc.h
>>> @@ -49,6 +49,8 @@
>>> enum tc_flower_reserved_prio {
>>> TC_RESERVED_PRIORITY_NONE,
>>> TC_RESERVED_PRIORITY_POLICE,
>>> + TC_RESERVED_PRIORITY_IPV4,
>>> + TC_RESERVED_PRIORITY_IPV6,
>>> __TC_RESERVED_PRIORITY_MAX
>>> };
>>> #define TC_RESERVED_PRIORITY_MAX (__TC_RESERVED_PRIORITY_MAX -1)
>>> --
>>> 2.38.0
>>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
