| Message ID | 20191210130129.29602-1-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev] dpif-netdev: Hold global port mutex while calling offload API. | expand |
On 12/10/2019 3:01 PM, Ilya Maximets wrote: > We changed datapath port lookup to netdev-offload API usage, but > forgot that port mutex was there not only to protect datapath > port hash map. It was there also as a workaround solution for > complete unsafety of netdev-offload-dpdk functions. > > Turning it back to fix the behaviour and adding a comment to prevent > removing it in the future unless netdev-offload-dpdk fixed. > > For the thread safety notice see the top of netdev-offload-dpdk.c. > > Fixes: 30115809da2e ("dpif-netdev: Use netdev-offload API for port lookup while offloading") > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > > Shame on me, I forgot about the documentation that I wrote 9 months ago. > > lib/dpif-netdev.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index 7ebf4ef87..c57458b62 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -2271,7 +2271,12 @@ mark_to_flow_disassociate(struct dp_netdev_pmd_thread *pmd, > > port = netdev_ports_get(in_port, pmd->dp->class); > if (port) { > + /* Taking a global 'port_mutex' to fulfill thread safety > + * restrictions for the netdev-offload-dpdk module. */ > + ovs_mutex_lock(&pmd->dp->port_mutex); > ret = netdev_flow_del(port, &flow->mega_ufid, NULL); > + ovs_mutex_unlock(&pmd->dp->port_mutex); > + Just remove this blank line. Acked-by: Eli Britstein <elibr@mellanox.com> > netdev_close(port); > } > > @@ -2415,10 +2420,14 @@ dp_netdev_flow_offload_put(struct dp_flow_offload_item *offload) > netdev_close(port); > goto err_free; > } > + /* Taking a global 'port_mutex' to fulfill thread safety restrictions for > + * the netdev-offload-dpdk module. */ > + ovs_mutex_lock(&pmd->dp->port_mutex); > ret = netdev_flow_put(port, &offload->match, > CONST_CAST(struct nlattr *, offload->actions), > offload->actions_len, &flow->mega_ufid, &info, > NULL); > + ovs_mutex_unlock(&pmd->dp->port_mutex); > netdev_close(port); > > if (ret) {
On 10.12.2019 16:21, Eli Britstein wrote: > > On 12/10/2019 3:01 PM, Ilya Maximets wrote: >> We changed datapath port lookup to netdev-offload API usage, but >> forgot that port mutex was there not only to protect datapath >> port hash map. It was there also as a workaround solution for >> complete unsafety of netdev-offload-dpdk functions. >> >> Turning it back to fix the behaviour and adding a comment to prevent >> removing it in the future unless netdev-offload-dpdk fixed. >> >> For the thread safety notice see the top of netdev-offload-dpdk.c. >> >> Fixes: 30115809da2e ("dpif-netdev: Use netdev-offload API for port lookup while offloading") >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> --- >> >> Shame on me, I forgot about the documentation that I wrote 9 months ago. >> >> lib/dpif-netdev.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >> index 7ebf4ef87..c57458b62 100644 >> --- a/lib/dpif-netdev.c >> +++ b/lib/dpif-netdev.c >> @@ -2271,7 +2271,12 @@ mark_to_flow_disassociate(struct dp_netdev_pmd_thread *pmd, >> >> port = netdev_ports_get(in_port, pmd->dp->class); >> if (port) { >> + /* Taking a global 'port_mutex' to fulfill thread safety >> + * restrictions for the netdev-offload-dpdk module. */ >> + ovs_mutex_lock(&pmd->dp->port_mutex); >> ret = netdev_flow_del(port, &flow->mega_ufid, NULL); >> + ovs_mutex_unlock(&pmd->dp->port_mutex); >> + > > Just remove this blank line. > > Acked-by: Eli Britstein <elibr@mellanox.com> Thanks. Applied to master.
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 7ebf4ef87..c57458b62 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -2271,7 +2271,12 @@ mark_to_flow_disassociate(struct dp_netdev_pmd_thread *pmd, port = netdev_ports_get(in_port, pmd->dp->class); if (port) { + /* Taking a global 'port_mutex' to fulfill thread safety + * restrictions for the netdev-offload-dpdk module. */ + ovs_mutex_lock(&pmd->dp->port_mutex); ret = netdev_flow_del(port, &flow->mega_ufid, NULL); + ovs_mutex_unlock(&pmd->dp->port_mutex); + netdev_close(port); } @@ -2415,10 +2420,14 @@ dp_netdev_flow_offload_put(struct dp_flow_offload_item *offload) netdev_close(port); goto err_free; } + /* Taking a global 'port_mutex' to fulfill thread safety restrictions for + * the netdev-offload-dpdk module. */ + ovs_mutex_lock(&pmd->dp->port_mutex); ret = netdev_flow_put(port, &offload->match, CONST_CAST(struct nlattr *, offload->actions), offload->actions_len, &flow->mega_ufid, &info, NULL); + ovs_mutex_unlock(&pmd->dp->port_mutex); netdev_close(port); if (ret) {
We changed datapath port lookup to netdev-offload API usage, but forgot that port mutex was there not only to protect datapath port hash map. It was there also as a workaround solution for complete unsafety of netdev-offload-dpdk functions. Turning it back to fix the behaviour and adding a comment to prevent removing it in the future unless netdev-offload-dpdk fixed. For the thread safety notice see the top of netdev-offload-dpdk.c. Fixes: 30115809da2e ("dpif-netdev: Use netdev-offload API for port lookup while offloading") Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- Shame on me, I forgot about the documentation that I wrote 9 months ago. lib/dpif-netdev.c | 9 +++++++++ 1 file changed, 9 insertions(+)