On 09/11/2018 12:30, Simon Horman wrote:
> On Thu, Nov 08, 2018 at 05:12:50AM -0500, Chris Mi wrote:
>> Currently, the ufid tc mapping is deleted in add_ufid_tc_mapping().
>> But if tc_replace_flower() failed, the old ufid tc mapping will not
>> be deleted. If another thread adds the same tc mapping successfully,
>> then there will be multiple mappings for the same ifindex, handle
>> and prio.
>>
>> Fixes: 9116730db ("netdev-tc-offloads: Add ufid to tc/netdev map")
>> Signed-off-by: Chris Mi <[email protected]>
>> Reviewed-by: Roi Dayan <[email protected]>
>> ---
>> lib/netdev-tc-offloads.c | 6 ++----
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
>> index 6f1fbe6..27a40bf 100644
>> --- a/lib/netdev-tc-offloads.c
>> +++ b/lib/netdev-tc-offloads.c
>> @@ -163,8 +163,7 @@ del_ufid_tc_mapping(const ovs_u128 *ufid)
>> ovs_mutex_unlock(&ufid_lock);
>> }
>>
>> -/* Add ufid entry to ufid_tc hashmap.
>> - * If entry exists already it will be replaced. */
>> +/* Add ufid entry to ufid_tc hashmap. */
>> static void
>> add_ufid_tc_mapping(const ovs_u128 *ufid, int prio, int handle,
>> struct netdev *netdev, int ifindex)
>> @@ -173,8 +172,6 @@ add_ufid_tc_mapping(const ovs_u128 *ufid, int prio, int
>> handle,
>> size_t tc_hash = hash_int(hash_int(prio, handle), ifindex);
>> struct ufid_tc_data *new_data = xzalloc(sizeof *new_data);
>>
>> - del_ufid_tc_mapping(ufid);
>> -
>> new_data->ufid = *ufid;
>> new_data->prio = prio;
>> new_data->handle = handle;
>> @@ -1303,6 +1300,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
>> *match,
>> if (handle && prio) {
>> VLOG_DBG_RL(&rl, "updating old handle: %d prio: %d", handle, prio);
>> tc_del_filter(ifindex, prio, handle, block_id);
>> + del_ufid_tc_mapping(ufid);
>> }
>>
>
> Hi Chris,
>
> Above the filter and then ufid mapping is deleted
> while in netdev_tc_flow_del the order is reversed.
>
> I wonder if we could make things more consistent using a wrapper
> as it seems that in netdev-tc-offloads.c these two operations always occur
> together.
Hi Simon,
I see in netdev_tc_flow_del the order is the same.
we call tc_del_filter and then del_ufid_tc_mapping.
we only have 2 places deleting filter and then ufid.
do you prefer to have a del wrapper for the 2 places?
Thanks,
Roi
> _______________________________________________
> dev mailing list
> [email protected]
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&data=02%7C01%7Croid%40mellanox.com%7Cf2a88683caa442d5f8b908d6462e7861%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636773562741076019&sdata=O%2FaSGgXfGva7ILu0P5oxQXuC%2BsQiP4rDmLtsyTHDh%2BY%3D&reserved=0
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
