diff mbox series

[ovs-dev,V3,1/4] dpif-netdev: Flush offload rules upon port deletion

Message ID 20201228101903.18186-2-elibr@nvidia.com
State Accepted
Headers show
Series netdev datapath flush offloaded flows | expand

Commit Message

Eli Britstein Dec. 28, 2020, 10:19 a.m. UTC 
When a port is deleted, flow deletion requests are posted, and the netdev
is removed from offload netdevs map. Following flow deletion handling may
be done after the netdev has already been removed from the offload
netdevs map, so the HW rule is not removed and the data object is not
freed (memory leak). Flush offload rules upon port deletion, and disable
pending handling of offloads to fix it.

Signed-off-by: Eli Britstein <elibr@nvidia.com>
Reviewed-by: Gaetan Rivet <gaetanr@nvidia.com>
---
 lib/dpif-netdev.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Tonghao Zhang Jan. 4, 2021, 1:33 p.m. UTC | #1 
On Tue, Dec 29, 2020 at 2:17 AM Eli Britstein <elibr@nvidia.com> wrote:
>
> When a port is deleted, flow deletion requests are posted, and the netdev
> is removed from offload netdevs map. Following flow deletion handling may
> be done after the netdev has already been removed from the offload
> netdevs map, so the HW rule is not removed and the data object is not
> freed (memory leak). Flush offload rules upon port deletion, and disable
> pending handling of offloads to fix it.
>
> Signed-off-by: Eli Britstein <elibr@nvidia.com>
> Reviewed-by: Gaetan Rivet <gaetanr@nvidia.com>
> ---
>  lib/dpif-netdev.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 300861ca5..3f0639fca 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -2281,6 +2281,8 @@ static void
>  do_del_port(struct dp_netdev *dp, struct dp_netdev_port *port)
>      OVS_REQUIRES(dp->port_mutex)
>  {
> +    netdev_flow_flush(port->netdev);
> +    netdev_uninit_flow_api(port->netdev);
Hi Eli
Why not invoke the netdev_flow_flush in netdev_ports_remove ?
I posted a patch for tc offload, but not accepted yet.
http://patchwork.ozlabs.org/project/openvswitch/patch/20200611103635.53367-1-xiangxia.m.yue@gmail.com/

>      hmap_remove(&dp->ports, &port->node);
>      seq_change(dp->port_seq);
>
> --
> 2.28.0.546.g385c171
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Eli Britstein Jan. 4, 2021, 1:46 p.m. UTC | #2 
On 1/4/2021 3:33 PM, Tonghao Zhang wrote:
> External email: Use caution opening links or attachments
>
>
> On Tue, Dec 29, 2020 at 2:17 AM Eli Britstein <elibr@nvidia.com> wrote:
>> When a port is deleted, flow deletion requests are posted, and the netdev
>> is removed from offload netdevs map. Following flow deletion handling may
>> be done after the netdev has already been removed from the offload
>> netdevs map, so the HW rule is not removed and the data object is not
>> freed (memory leak). Flush offload rules upon port deletion, and disable
>> pending handling of offloads to fix it.
>>
>> Signed-off-by: Eli Britstein <elibr@nvidia.com>
>> Reviewed-by: Gaetan Rivet <gaetanr@nvidia.com>
>> ---
>>   lib/dpif-netdev.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 300861ca5..3f0639fca 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -2281,6 +2281,8 @@ static void
>>   do_del_port(struct dp_netdev *dp, struct dp_netdev_port *port)
>>       OVS_REQUIRES(dp->port_mutex)
>>   {
>> +    netdev_flow_flush(port->netdev);
>> +    netdev_uninit_flow_api(port->netdev);
> Hi Eli
> Why not invoke the netdev_flow_flush in netdev_ports_remove ?
> I posted a patch for tc offload, but not accepted yet.
> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatchwork.ozlabs.org%2Fproject%2Fopenvswitch%2Fpatch%2F20200611103635.53367-1-xiangxia.m.yue%40gmail.com%2F&amp;data=04%7C01%7Celibr%40nvidia.com%7C53c2ee7dfde349ce938908d8b0b56f01%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637453640601739470%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=AyQckaIcBfDeGHAJSe43BNAzpCu2q9U3xWbYGzV9hbY%3D&amp;reserved=0

Hi Tonghao,

Thanks for your review.

In dpif-netdev, we should be under port_mutex lock. It is not the case 
in your patch. Also, netdev_uninit_flow_api is called here to make sure 
no more offload requests are handled for this port, even the pending 
ones in the queue. netdev_flow_flush won't do anything if called afterwards.

Thanks,

Eli

>
>>       hmap_remove(&dp->ports, &port->node);
>>       seq_change(dp->port_seq);
>>
>> --
>> 2.28.0.546.g385c171
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&amp;data=04%7C01%7Celibr%40nvidia.com%7C53c2ee7dfde349ce938908d8b0b56f01%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637453640601739470%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=YxomBNTgR5RK8jGHfjQ83UlkjrpXJOJx1ZtT1r67PKg%3D&amp;reserved=0
>
>
> --
> Best regards, Tonghao
Emma Finn Jan. 13, 2021, 8:52 a.m. UTC | #3 
> -----Original Message-----
> From: dev <ovs-dev-bounces@openvswitch.org> On Behalf Of Eli Britstein
> Sent: Monday 4 January 2021 13:46
> To: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> Cc: ovs dev <dev@openvswitch.org>; Gaetan Rivet <gaetanr@nvidia.com>; Ilya
> Maximets <i.maximets@ovn.org>
> Subject: Re: [ovs-dev] [PATCH V3 1/4] dpif-netdev: Flush offload rules upon port
> deletion
> 
> 
> On 1/4/2021 3:33 PM, Tonghao Zhang wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On Tue, Dec 29, 2020 at 2:17 AM Eli Britstein <elibr@nvidia.com> wrote:
> >> When a port is deleted, flow deletion requests are posted, and the
> >> netdev is removed from offload netdevs map. Following flow deletion
> >> handling may be done after the netdev has already been removed from
> >> the offload netdevs map, so the HW rule is not removed and the data
> >> object is not freed (memory leak). Flush offload rules upon port
> >> deletion, and disable pending handling of offloads to fix it.
> >>
> >> Signed-off-by: Eli Britstein <elibr@nvidia.com>
> >> Reviewed-by: Gaetan Rivet <gaetanr@nvidia.com>
> >> ---
> >>   lib/dpif-netdev.c | 2 ++
> >>   1 file changed, 2 insertions(+)
> >>
> >> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index
> >> 300861ca5..3f0639fca 100644
> >> --- a/lib/dpif-netdev.c
> >> +++ b/lib/dpif-netdev.c
> >> @@ -2281,6 +2281,8 @@ static void
> >>   do_del_port(struct dp_netdev *dp, struct dp_netdev_port *port)
> >>       OVS_REQUIRES(dp->port_mutex)
> >>   {
> >> +    netdev_flow_flush(port->netdev);
> >> +    netdev_uninit_flow_api(port->netdev);
> > Hi Eli
> > Why not invoke the netdev_flow_flush in netdev_ports_remove ?
> > I posted a patch for tc offload, but not accepted yet.
> > https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatch
> >
> work.ozlabs.org%2Fproject%2Fopenvswitch%2Fpatch%2F20200611103635.5336
> 7
> > -1-
> xiangxia.m.yue%40gmail.com%2F&amp;data=04%7C01%7Celibr%40nvidia.com
> >
> %7C53c2ee7dfde349ce938908d8b0b56f01%7C43083d15727340c1b7db39efd9c
> cc17a
> >
> %7C0%7C0%7C637453640601739470%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi
> MC4wLjAw
> >
> MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdat
> a=Ay
> > QckaIcBfDeGHAJSe43BNAzpCu2q9U3xWbYGzV9hbY%3D&amp;reserved=0
> 
> Hi Tonghao,
> 
> Thanks for your review.
> 
> In dpif-netdev, we should be under port_mutex lock. It is not the case in your
> patch. Also, netdev_uninit_flow_api is called here to make sure no more
> offload requests are handled for this port, even the pending ones in the queue.
> netdev_flow_flush won't do anything if called afterwards.
> 
> Thanks,
> 
> Eli
> 

LGTM.
Tested with Intel XL710 Devices.
Acked-by: Emma Finn <emma.finn@intel.com>
Tested-by: Emma Finn <emma.finn@intel.com>

> >
> >>       hmap_remove(&dp->ports, &port->node);
> >>       seq_change(dp->port_seq);
> >>
> >> --
> >> 2.28.0.546.g385c171
> >>
> >> _______________________________________________
> >> dev mailing list
> >> dev@openvswitch.org
> >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmai
> >> l.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-
> dev&amp;data=04%7C01%7Ce
> >>
> libr%40nvidia.com%7C53c2ee7dfde349ce938908d8b0b56f01%7C43083d15727
> 340
> >>
> c1b7db39efd9ccc17a%7C0%7C0%7C637453640601739470%7CUnknown%7CTW
> FpbGZsb
> >>
> 3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%
> 3D
> >>
> %7C1000&amp;sdata=YxomBNTgR5RK8jGHfjQ83UlkjrpXJOJx1ZtT1r67PKg%3D&
> amp;
> >> reserved=0
> >
> >
> > --
> > Best regards, Tonghao
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff mbox series

Patch

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 300861ca5..3f0639fca 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -2281,6 +2281,8 @@  static void
 do_del_port(struct dp_netdev *dp, struct dp_netdev_port *port)
     OVS_REQUIRES(dp->port_mutex)
 {
+    netdev_flow_flush(port->netdev);
+    netdev_uninit_flow_api(port->netdev);
     hmap_remove(&dp->ports, &port->node);
     seq_change(dp->port_seq);