[ovs-dev] netdev-tc-offloads: Delete ufid tc mapping in the right place

Message ID 1541671970-28973-1-git-send-email-chrism@mellanox.com
State New
Headers show
Series
  • [ovs-dev] netdev-tc-offloads: Delete ufid tc mapping in the right place
Related show

Commit Message

Chris Mi Nov. 8, 2018, 10:12 a.m.
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 <chrism@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
---
 lib/netdev-tc-offloads.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Simon Horman Nov. 9, 2018, 10:30 a.m. | #1
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 <chrism@mellanox.com>
> Reviewed-by: Roi Dayan <roid@mellanox.com>
> ---
>  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.
Roi Dayan Nov. 11, 2018, 8:18 a.m. | #2
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 <chrism@mellanox.com>
>> Reviewed-by: Roi Dayan <roid@mellanox.com>
>> ---
>>  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
> dev@openvswitch.org
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&amp;data=02%7C01%7Croid%40mellanox.com%7Cf2a88683caa442d5f8b908d6462e7861%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636773562741076019&amp;sdata=O%2FaSGgXfGva7ILu0P5oxQXuC%2BsQiP4rDmLtsyTHDh%2BY%3D&amp;reserved=0
>
Chris Mi Nov. 12, 2018, 2:10 a.m. | #3
> -----Original Message-----
> From: Simon Horman [mailto:simon.horman@netronome.com]
> Sent: Friday, November 9, 2018 6:31 PM
> To: Chris Mi <chrism@mellanox.com>
> Cc: dev@openvswitch.org
> Subject: Re: [ovs-dev] netdev-tc-offloads: Delete ufid tc mapping in the right
> place
> 
> 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 <chrism@mellanox.com>
> > Reviewed-by: Roi Dayan <roid@mellanox.com>
> > ---
> >  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,

Thanks for your suggestion. Above comment has been addressed in v2.
Please help review again.

Thanks,
Chris

Patch

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);
     }
 
     if (!prio) {