| Message ID | 20191208132304.15521-9-elibr@mellanox.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | netdev datapath actions offload | expand |
Hi, cmap support multiple readers and one writer. since all write operations are performed in the offload thread, why need a mutex here? Maybe I miss some patches. Eli Britstein <elibr@mellanox.com> 于2019年12月8日周日 下午9:23写道： > > Flow deletion and dumping for statistics collection are called from > different threads. As a pre-step towards collecting HW statistics, > protect the UFID map by mutex to make it thread safe. > > Signed-off-by: Eli Britstein <elibr@mellanox.com> > --- > lib/netdev-offload-dpdk.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c > index b2ec05cec..5568400b6 100644 > --- a/lib/netdev-offload-dpdk.c > +++ b/lib/netdev-offload-dpdk.c > @@ -51,6 +51,7 @@ static struct vlog_rate_limit error_rl = VLOG_RATE_LIMIT_INIT(100, 5); > * A mapping from ufid to dpdk rte_flow. > */ > static struct cmap ufid_to_rte_flow = CMAP_INITIALIZER; > +static struct ovs_mutex ufid_map_mutex = OVS_MUTEX_INITIALIZER; > > struct ufid_to_rte_flow_data { > struct cmap_node node; > @@ -630,8 +631,11 @@ netdev_offload_dpdk_destroy_flow(struct netdev *netdev, > struct rte_flow *rte_flow) > { > struct rte_flow_error error; > - int ret = netdev_dpdk_rte_flow_destroy(netdev, rte_flow, &error); > + int ret; > + > + ovs_mutex_lock(&ufid_map_mutex); > > + ret = netdev_dpdk_rte_flow_destroy(netdev, rte_flow, &error); > if (ret == 0) { > ufid_to_rte_flow_disassociate(ufid); > VLOG_DBG("%s: removed rte flow %p associated with ufid " UUID_FMT "\n", > @@ -642,6 +646,7 @@ netdev_offload_dpdk_destroy_flow(struct netdev *netdev, > netdev_get_name(netdev), error.message, error.type); > } > > + ovs_mutex_unlock(&ufid_map_mutex); > return ret; > } > > -- > 2.14.5 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev -- hepeng ICT
On 12/9/2019 9:15 AM, 贺鹏 wrote: > Hi, > > > cmap support multiple readers and one writer. > since all write operations are performed in the offload thread, > why need a mutex here? Maybe I miss some patches. Collecting statistics using netdev_flow_get in patch 10/20 is another thread. There, it gets the rte_flow, but if not protected it might get destroyed by the offload thread before it uses it. > > > > Eli Britstein <elibr@mellanox.com> 于2019年12月8日周日 下午9:23写道： > >> Flow deletion and dumping for statistics collection are called from >> different threads. As a pre-step towards collecting HW statistics, >> protect the UFID map by mutex to make it thread safe. >> >> Signed-off-by: Eli Britstein <elibr@mellanox.com> >> --- >> lib/netdev-offload-dpdk.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c >> index b2ec05cec..5568400b6 100644 >> --- a/lib/netdev-offload-dpdk.c >> +++ b/lib/netdev-offload-dpdk.c >> @@ -51,6 +51,7 @@ static struct vlog_rate_limit error_rl = VLOG_RATE_LIMIT_INIT(100, 5); >> * A mapping from ufid to dpdk rte_flow. >> */ >> static struct cmap ufid_to_rte_flow = CMAP_INITIALIZER; >> +static struct ovs_mutex ufid_map_mutex = OVS_MUTEX_INITIALIZER; >> >> struct ufid_to_rte_flow_data { >> struct cmap_node node; >> @@ -630,8 +631,11 @@ netdev_offload_dpdk_destroy_flow(struct netdev *netdev, >> struct rte_flow *rte_flow) >> { >> struct rte_flow_error error; >> - int ret = netdev_dpdk_rte_flow_destroy(netdev, rte_flow, &error); >> + int ret; >> + >> + ovs_mutex_lock(&ufid_map_mutex); >> >> + ret = netdev_dpdk_rte_flow_destroy(netdev, rte_flow, &error); >> if (ret == 0) { >> ufid_to_rte_flow_disassociate(ufid); >> VLOG_DBG("%s: removed rte flow %p associated with ufid " UUID_FMT "\n", >> @@ -642,6 +646,7 @@ netdev_offload_dpdk_destroy_flow(struct netdev *netdev, >> netdev_get_name(netdev), error.message, error.type); >> } >> >> + ovs_mutex_unlock(&ufid_map_mutex); >> return ret; >> } >> >> -- >> 2.14.5 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&data=02%7C01%7Celibr%40mellanox.com%7C9af2f119de544768711408d77c779a66%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637114725449215533&sdata=VyrLUh6%2BP0ne9dXUSDjME31NBj0tHTV%2FEciNZY%2Baugo%3D&reserved=0 > > > -- > hepeng > ICT
Got it. You're correct. Eli Britstein <elibr@mellanox.com> 于2019年12月9日周一 下午3:21写道： > > > On 12/9/2019 9:15 AM, 贺鹏 wrote: > > Hi, > > > > > > cmap support multiple readers and one writer. > > since all write operations are performed in the offload thread, > > why need a mutex here? Maybe I miss some patches. > Collecting statistics using netdev_flow_get in patch 10/20 is another > thread. There, it gets the rte_flow, but if not protected it might get > destroyed by the offload thread before it uses it. > > > > > > > > Eli Britstein <elibr@mellanox.com> 于2019年12月8日周日 下午9:23写道： > > > >> Flow deletion and dumping for statistics collection are called from > >> different threads. As a pre-step towards collecting HW statistics, > >> protect the UFID map by mutex to make it thread safe. > >> > >> Signed-off-by: Eli Britstein <elibr@mellanox.com> > >> --- > >> lib/netdev-offload-dpdk.c | 7 ++++++- > >> 1 file changed, 6 insertions(+), 1 deletion(-) > >> > >> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c > >> index b2ec05cec..5568400b6 100644 > >> --- a/lib/netdev-offload-dpdk.c > >> +++ b/lib/netdev-offload-dpdk.c > >> @@ -51,6 +51,7 @@ static struct vlog_rate_limit error_rl = VLOG_RATE_LIMIT_INIT(100, 5); > >> * A mapping from ufid to dpdk rte_flow. > >> */ > >> static struct cmap ufid_to_rte_flow = CMAP_INITIALIZER; > >> +static struct ovs_mutex ufid_map_mutex = OVS_MUTEX_INITIALIZER; > >> > >> struct ufid_to_rte_flow_data { > >> struct cmap_node node; > >> @@ -630,8 +631,11 @@ netdev_offload_dpdk_destroy_flow(struct netdev *netdev, > >> struct rte_flow *rte_flow) > >> { > >> struct rte_flow_error error; > >> - int ret = netdev_dpdk_rte_flow_destroy(netdev, rte_flow, &error); > >> + int ret; > >> + > >> + ovs_mutex_lock(&ufid_map_mutex); > >> > >> + ret = netdev_dpdk_rte_flow_destroy(netdev, rte_flow, &error); > >> if (ret == 0) { > >> ufid_to_rte_flow_disassociate(ufid); > >> VLOG_DBG("%s: removed rte flow %p associated with ufid " UUID_FMT "\n", > >> @@ -642,6 +646,7 @@ netdev_offload_dpdk_destroy_flow(struct netdev *netdev, > >> netdev_get_name(netdev), error.message, error.type); > >> } > >> > >> + ovs_mutex_unlock(&ufid_map_mutex); > >> return ret; > >> } > >> > >> -- > >> 2.14.5 > >> > >> _______________________________________________ > >> dev mailing list > >> dev@openvswitch.org > >> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&data=02%7C01%7Celibr%40mellanox.com%7C9af2f119de544768711408d77c779a66%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637114725449215533&sdata=VyrLUh6%2BP0ne9dXUSDjME31NBj0tHTV%2FEciNZY%2Baugo%3D&reserved=0 > > > > > > -- > > hepeng > > ICT
On 08.12.2019 14:22, Eli Britstein wrote: > Flow deletion and dumping for statistics collection are called from > different threads. As a pre-step towards collecting HW statistics, > protect the UFID map by mutex to make it thread safe. > > Signed-off-by: Eli Britstein <elibr@mellanox.com> > --- > lib/netdev-offload-dpdk.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > This patch is not needed as you have to hold global datapath port mutex while calling netdev-offload-dpdk functions. This module is not thread safe at all. See https://patchwork.ozlabs.org/patch/1207077/ . Best regards, Ilya Maximets.
On 12/10/2019 4:53 PM, Ilya Maximets wrote: > On 08.12.2019 14:22, Eli Britstein wrote: >> Flow deletion and dumping for statistics collection are called from >> different threads. As a pre-step towards collecting HW statistics, >> protect the UFID map by mutex to make it thread safe. >> >> Signed-off-by: Eli Britstein <elibr@mellanox.com> >> --- >> lib/netdev-offload-dpdk.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> > This patch is not needed as you have to hold global datapath port > mutex while calling netdev-offload-dpdk functions. This module > is not thread safe at all. > > See https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.ozlabs.org%2Fpatch%2F1207077%2F&data=02%7C01%7Celibr%40mellanox.com%7C25748aeece0d4a07a10408d77d80b937%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637115864133577751&sdata=ndq%2Fda30D1ih4AaUmLvfFmVwXfcTO9hr5Y7wyM6KOzs%3D&reserved=0 . That's right, but in next commits, reading the stats using netdev_flow_get is done from another thread. This commit is a pre-step towards it. see the commit msg. > > Best regards, Ilya Maximets.
On 10.12.2019 15:56, Eli Britstein wrote: > > On 12/10/2019 4:53 PM, Ilya Maximets wrote: >> On 08.12.2019 14:22, Eli Britstein wrote: >>> Flow deletion and dumping for statistics collection are called from >>> different threads. As a pre-step towards collecting HW statistics, >>> protect the UFID map by mutex to make it thread safe. >>> >>> Signed-off-by: Eli Britstein <elibr@mellanox.com> >>> --- >>> lib/netdev-offload-dpdk.c | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >> This patch is not needed as you have to hold global datapath port >> mutex while calling netdev-offload-dpdk functions. This module >> is not thread safe at all. >> >> See https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.ozlabs.org%2Fpatch%2F1207077%2F&data=02%7C01%7Celibr%40mellanox.com%7C25748aeece0d4a07a10408d77d80b937%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637115864133577751&sdata=ndq%2Fda30D1ih4AaUmLvfFmVwXfcTO9hr5Y7wyM6KOzs%3D&reserved=0 . > That's right, but in next commits, reading the stats using > netdev_flow_get is done from another thread. This commit is a pre-step > towards it. see the commit msg. You have to hold dp->port_mutex while calling netdev_flow_get().
On 12/10/2019 5:03 PM, Ilya Maximets wrote: > On 10.12.2019 15:56, Eli Britstein wrote: >> On 12/10/2019 4:53 PM, Ilya Maximets wrote: >>> On 08.12.2019 14:22, Eli Britstein wrote: >>>> Flow deletion and dumping for statistics collection are called from >>>> different threads. As a pre-step towards collecting HW statistics, >>>> protect the UFID map by mutex to make it thread safe. >>>> >>>> Signed-off-by: Eli Britstein <elibr@mellanox.com> >>>> --- >>>> lib/netdev-offload-dpdk.c | 7 ++++++- >>>> 1 file changed, 6 insertions(+), 1 deletion(-) >>>> >>> This patch is not needed as you have to hold global datapath port >>> mutex while calling netdev-offload-dpdk functions. This module >>> is not thread safe at all. >>> >>> See https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.ozlabs.org%2Fpatch%2F1207077%2F&data=02%7C01%7Celibr%40mellanox.com%7C7882b1cc0bde4a628f1308d77d821054%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637115869897963087&sdata=WYDwU88eZXD3iSNB9RUyyoNRqC4c8WJhr06%2Fh70%2Fvxw%3D&reserved=0 . >> That's right, but in next commits, reading the stats using >> netdev_flow_get is done from another thread. This commit is a pre-step >> towards it. see the commit msg. > You have to hold dp->port_mutex while calling netdev_flow_get(). OK.
diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c index b2ec05cec..5568400b6 100644 --- a/lib/netdev-offload-dpdk.c +++ b/lib/netdev-offload-dpdk.c @@ -51,6 +51,7 @@ static struct vlog_rate_limit error_rl = VLOG_RATE_LIMIT_INIT(100, 5); * A mapping from ufid to dpdk rte_flow. */ static struct cmap ufid_to_rte_flow = CMAP_INITIALIZER; +static struct ovs_mutex ufid_map_mutex = OVS_MUTEX_INITIALIZER; struct ufid_to_rte_flow_data { struct cmap_node node; @@ -630,8 +631,11 @@ netdev_offload_dpdk_destroy_flow(struct netdev *netdev, struct rte_flow *rte_flow) { struct rte_flow_error error; - int ret = netdev_dpdk_rte_flow_destroy(netdev, rte_flow, &error); + int ret; + + ovs_mutex_lock(&ufid_map_mutex); + ret = netdev_dpdk_rte_flow_destroy(netdev, rte_flow, &error); if (ret == 0) { ufid_to_rte_flow_disassociate(ufid); VLOG_DBG("%s: removed rte flow %p associated with ufid " UUID_FMT "\n", @@ -642,6 +646,7 @@ netdev_offload_dpdk_destroy_flow(struct netdev *netdev, netdev_get_name(netdev), error.message, error.type); } + ovs_mutex_unlock(&ufid_map_mutex); return ret; }
Flow deletion and dumping for statistics collection are called from different threads. As a pre-step towards collecting HW statistics, protect the UFID map by mutex to make it thread safe. Signed-off-by: Eli Britstein <elibr@mellanox.com> --- lib/netdev-offload-dpdk.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)