Message ID | 1541671970-28973-1-git-send-email-chrism@mellanox.com |
---|---|
State | Superseded |
Delegated to: | Simon Horman |
Headers | show |
Series | [ovs-dev] netdev-tc-offloads: Delete ufid tc mapping in the right place | expand |
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.
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&data=02%7C01%7Croid%40mellanox.com%7Cf2a88683caa442d5f8b908d6462e7861%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636773562741076019&sdata=O%2FaSGgXfGva7ILu0P5oxQXuC%2BsQiP4rDmLtsyTHDh%2BY%3D&reserved=0 >
> -----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
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) {