mbox series

[ovs-dev,v1,0/2] Add netdev-dpdk support for ingress and egress pkts policer.

Message ID SY4PR01MB84387760C6401DF07F40B137CD8D9@SY4PR01MB8438.ausprd01.prod.outlook.com
Headers show
Series Add netdev-dpdk support for ingress and egress pkts policer. | expand

Message

miter April 2, 2023, 9:30 a.m. UTC
From: Lin Huang <linhuang@ruijie.com.cn>

This series patch set adds ingress and egress pkts policer support for netdev-dpdk
device.

Lin Huang (2):
  netdev-dpdk: add netdev-dpdk egress pkts policer.
  netdev-dpdk: add netdev-dpdk ingress pkts policer.

 lib/netdev-dpdk.c    | 233 ++++++++++++++++++++++++++++++++++++++++++-
 vswitchd/vswitch.xml |  32 ++++++
 2 files changed, 261 insertions(+), 4 deletions(-)

Comments

miter April 4, 2023, 1:46 p.m. UTC | #1
Hi Ilya,

Pls review my code. 

I want to add a new policer which is pps-based in netdev-dpdk module.
The policer is divided into ingress and egress part. Both use the ovs native tocken bucket library as the counter.
Compared to bandwidth-based policers, the pps-based policer is more effective at combating low-and-slow types of DDoS attacks.

This new ingress policer is configured using the "policer_kpkts_rate" and "policer_kpkts_burst" configuration statement. Here is the example.
ovs-vsctl set interface dpdk0 policer_kpkts_rate=2
ovs-vsctl set interface dpdk0 policer_kpkts_burst=2

This new egress policer is configured using the " egress-pkts-policer " configuration statement. Here is the example.
ovs-vsctl set port vhost-user0 qos=@newqos -- \
    --id=@newqos create qos type=egress-pkts-policer other-config:pkts_rate=2048 \
    other-config:pkts_burst=2048`

Thanks a lot.

> -----Original Message-----
> From: lin huang <miterv@outlook.com>
> Sent: Sunday, April 2, 2023 5:30 PM
> To: dev@openvswitch.org; i.maximets@ovn.org
> Cc: Lin Huang <linhuang@ruijie.com.cn>
> Subject: [PATCH v1 0/2] Add netdev-dpdk support for ingress and egress pkts
> policer.
> 
> From: Lin Huang <linhuang@ruijie.com.cn>
> 
> This series patch set adds ingress and egress pkts policer support for
> netdev-dpdk
> device.
> 
> Lin Huang (2):
>   netdev-dpdk: add netdev-dpdk egress pkts policer.
>   netdev-dpdk: add netdev-dpdk ingress pkts policer.
> 
>  lib/netdev-dpdk.c    | 233
> ++++++++++++++++++++++++++++++++++++++++++-
>  vswitchd/vswitch.xml |  32 ++++++
>  2 files changed, 261 insertions(+), 4 deletions(-)
> 
> --
> 2.37.1.windows.1
Simon Horman April 5, 2023, 12:28 p.m. UTC | #2
On Tue, Apr 04, 2023 at 01:46:05PM +0000, lin huang wrote:
> Hi Ilya,
> 
> Pls review my code. 
> 
> I want to add a new policer which is pps-based in netdev-dpdk module.
> The policer is divided into ingress and egress part. Both use the ovs native tocken bucket library as the counter.
> Compared to bandwidth-based policers, the pps-based policer is more effective at combating low-and-slow types of DDoS attacks.
> 
> This new ingress policer is configured using the "policer_kpkts_rate" and "policer_kpkts_burst" configuration statement. Here is the example.
> ovs-vsctl set interface dpdk0 policer_kpkts_rate=2
> ovs-vsctl set interface dpdk0 policer_kpkts_burst=2
> 
> This new egress policer is configured using the " egress-pkts-policer " configuration statement. Here is the example.
> ovs-vsctl set port vhost-user0 qos=@newqos -- \
>     --id=@newqos create qos type=egress-pkts-policer other-config:pkts_rate=2048 \
>     other-config:pkts_burst=2048`

I have some high level questions about this.

1. For ingress there is, as you mention above,
   already options at the ovs-vsctl for port-based PPS policing.

   Did you consider making the egress options you are adding consistent
   with the existing ingress options?
   f.e.  egress_policing_kpkts_rate and egress_policing_kpkts_rate.

2. Metering operates on egress, is a policer, and can support PPS.
   Did you consider using this instead of adding a new egress policer?

   There is an upstreamed implementation of this for upstreamed
   for the kernel datapath with and without TC HW offload.
miter April 5, 2023, 2:07 p.m. UTC | #3
Hi Simon,

: ) 
Thanks for your reply.

There are some explanations about your comments.

Best regards, Huang Lin.

> -----Original Message-----
> From: Simon Horman <simon.horman@corigine.com>
> Sent: Wednesday, April 5, 2023 8:28 PM
> To: lin huang <miterv@outlook.com>
> Cc: dev@openvswitch.org; i.maximets@ovn.org
> Subject: Re: [ovs-dev] [PATCH v1 0/2] Add netdev-dpdk support for ingress and
> egress pkts policer.
> 
> On Tue, Apr 04, 2023 at 01:46:05PM +0000, lin huang wrote:
> > Hi Ilya,
> >
> > Pls review my code.
> >
> > I want to add a new policer which is pps-based in netdev-dpdk module.
> > The policer is divided into ingress and egress part. Both use the ovs native
> tocken bucket library as the counter.
> > Compared to bandwidth-based policers, the pps-based policer is more
> effective at combating low-and-slow types of DDoS attacks.
> >
> > This new ingress policer is configured using the "policer_kpkts_rate" and
> "policer_kpkts_burst" configuration statement. Here is the example.
> > ovs-vsctl set interface dpdk0 policer_kpkts_rate=2
> > ovs-vsctl set interface dpdk0 policer_kpkts_burst=2
> >
> > This new egress policer is configured using the " egress-pkts-policer "
> configuration statement. Here is the example.
> > ovs-vsctl set port vhost-user0 qos=@newqos -- \
> >     --id=@newqos create qos type=egress-pkts-policer
> other-config:pkts_rate=2048 \
> >     other-config:pkts_burst=2048`
> 
> I have some high level questions about this.
> 
> 1. For ingress there is, as you mention above,
>    already options at the ovs-vsctl for port-based PPS policing.
> 
>    Did you consider making the egress options you are adding consistent
>    with the existing ingress options?
>    f.e.  egress_policing_kpkts_rate and egress_policing_kpkts_rate.
	
	Yes we can add new egress options in interface. But this work will conflict the 
    userspace datapath egress QoS (egress policing) framework. 
	The framework needs to set up a QoS instance which is configured by cir/cbs/pps... to a specific port.
	So, I add a new egress policer called pkts-policer.

> 
> 2. Metering operates on egress, is a policer, and can support PPS.
>    Did you consider using this instead of adding a new egress policer?

	In my opinion, meter table is used in flow rate limit not in interface traffic.
    Interface policer is different from meter policer which you can police a specific flow.
	The new egress policer is just in userspace datapath not in kernel datapath.
	Does the new policer involve the kernel datapath?

> 
>    There is an upstreamed implementation of this for upstreamed
>    for the kernel datapath with and without TC HW offload.
Simon Horman April 6, 2023, 6:38 a.m. UTC | #4
On Wed, Apr 05, 2023 at 02:07:42PM +0000, lin huang wrote:
> Hi Simon,
> 
> : ) 
> Thanks for your reply.
> 
> There are some explanations about your comments.
> 
> Best regards, Huang Lin.
> 
> > -----Original Message-----
> > From: Simon Horman <simon.horman@corigine.com>
> > Sent: Wednesday, April 5, 2023 8:28 PM
> > To: lin huang <miterv@outlook.com>
> > Cc: dev@openvswitch.org; i.maximets@ovn.org
> > Subject: Re: [ovs-dev] [PATCH v1 0/2] Add netdev-dpdk support for ingress and
> > egress pkts policer.
> > 
> > On Tue, Apr 04, 2023 at 01:46:05PM +0000, lin huang wrote:
> > > Hi Ilya,
> > >
> > > Pls review my code.
> > >
> > > I want to add a new policer which is pps-based in netdev-dpdk module.
> > > The policer is divided into ingress and egress part. Both use the ovs native
> > tocken bucket library as the counter.
> > > Compared to bandwidth-based policers, the pps-based policer is more
> > effective at combating low-and-slow types of DDoS attacks.
> > >
> > > This new ingress policer is configured using the "policer_kpkts_rate" and
> > "policer_kpkts_burst" configuration statement. Here is the example.
> > > ovs-vsctl set interface dpdk0 policer_kpkts_rate=2
> > > ovs-vsctl set interface dpdk0 policer_kpkts_burst=2
> > >
> > > This new egress policer is configured using the " egress-pkts-policer "
> > configuration statement. Here is the example.
> > > ovs-vsctl set port vhost-user0 qos=@newqos -- \
> > >     --id=@newqos create qos type=egress-pkts-policer
> > other-config:pkts_rate=2048 \
> > >     other-config:pkts_burst=2048`
> > 
> > I have some high level questions about this.
> > 
> > 1. For ingress there is, as you mention above,
> >    already options at the ovs-vsctl for port-based PPS policing.
> > 
> >    Did you consider making the egress options you are adding consistent
> >    with the existing ingress options?
> >    f.e.  egress_policing_kpkts_rate and egress_policing_kpkts_rate.
> 	
> 	Yes we can add new egress options in interface. But this work will conflict the 
>     userspace datapath egress QoS (egress policing) framework. 
> 	The framework needs to set up a QoS instance which is configured by cir/cbs/pps... to a specific port.
> 	So, I add a new egress policer called pkts-policer.

My point was mainly that for ingress we use 'kpkts'
But your proposal for egress uses 'pkts'.
I think they should be the same.

> > 2. Metering operates on egress, is a policer, and can support PPS.
> >    Did you consider using this instead of adding a new egress policer?
> 
> 	In my opinion, meter table is used in flow rate limit not in interface traffic.
>     Interface policer is different from meter policer which you can police a specific flow.
> 	The new egress policer is just in userspace datapath not in kernel datapath.
> 	Does the new policer involve the kernel datapath?
> 
> > 
> >    There is an upstreamed implementation of this for upstreamed
> >    for the kernel datapath with and without TC HW offload.

My point is that, in my experience, metering covers your use-case as I
understand it. I'd be interested to hear of an example where it does not.
Ilya Maximets April 6, 2023, 12:13 p.m. UTC | #5
On 4/6/23 08:38, Simon Horman wrote:
> On Wed, Apr 05, 2023 at 02:07:42PM +0000, lin huang wrote:
>> Hi Simon,
>>
>> : ) 
>> Thanks for your reply.
>>
>> There are some explanations about your comments.
>>
>> Best regards, Huang Lin.
>>
>>> -----Original Message-----
>>> From: Simon Horman <simon.horman@corigine.com>
>>> Sent: Wednesday, April 5, 2023 8:28 PM
>>> To: lin huang <miterv@outlook.com>
>>> Cc: dev@openvswitch.org; i.maximets@ovn.org
>>> Subject: Re: [ovs-dev] [PATCH v1 0/2] Add netdev-dpdk support for ingress and
>>> egress pkts policer.
>>>
>>> On Tue, Apr 04, 2023 at 01:46:05PM +0000, lin huang wrote:
>>>> Hi Ilya,
>>>>
>>>> Pls review my code.
>>>>
>>>> I want to add a new policer which is pps-based in netdev-dpdk module.
>>>> The policer is divided into ingress and egress part. Both use the ovs native
>>> tocken bucket library as the counter.
>>>> Compared to bandwidth-based policers, the pps-based policer is more
>>> effective at combating low-and-slow types of DDoS attacks.
>>>>
>>>> This new ingress policer is configured using the "policer_kpkts_rate" and
>>> "policer_kpkts_burst" configuration statement. Here is the example.
>>>> ovs-vsctl set interface dpdk0 policer_kpkts_rate=2
>>>> ovs-vsctl set interface dpdk0 policer_kpkts_burst=2
>>>>
>>>> This new egress policer is configured using the " egress-pkts-policer "
>>> configuration statement. Here is the example.
>>>> ovs-vsctl set port vhost-user0 qos=@newqos -- \
>>>>     --id=@newqos create qos type=egress-pkts-policer
>>> other-config:pkts_rate=2048 \
>>>>     other-config:pkts_burst=2048`
>>>
>>> I have some high level questions about this.
>>>
>>> 1. For ingress there is, as you mention above,
>>>    already options at the ovs-vsctl for port-based PPS policing.
>>>
>>>    Did you consider making the egress options you are adding consistent
>>>    with the existing ingress options?
>>>    f.e.  egress_policing_kpkts_rate and egress_policing_kpkts_rate.
>> 	
>> 	Yes we can add new egress options in interface. But this work will conflict the 
>>     userspace datapath egress QoS (egress policing) framework. 
>> 	The framework needs to set up a QoS instance which is configured by cir/cbs/pps... to a specific port.
>> 	So, I add a new egress policer called pkts-policer.
> 
> My point was mainly that for ingress we use 'kpkts'
> But your proposal for egress uses 'pkts'.
> I think they should be the same.

I had the same thought, so pointed that out in my review for the patch.
I think, we agreed to change to kilo-. The math will also become less
confusing.

> 
>>> 2. Metering operates on egress, is a policer, and can support PPS.
>>>    Did you consider using this instead of adding a new egress policer?
>>
>> 	In my opinion, meter table is used in flow rate limit not in interface traffic.
>>     Interface policer is different from meter policer which you can police a specific flow.
>> 	The new egress policer is just in userspace datapath not in kernel datapath.
>> 	Does the new policer involve the kernel datapath?
>>
>>>
>>>    There is an upstreamed implementation of this for upstreamed
>>>    for the kernel datapath with and without TC HW offload.
> 
> My point is that, in my experience, metering covers your use-case as I
> understand it. I'd be interested to hear of an example where it does not.

I suppose, it's useful for those who are using OVS as a simple learning
switch and don't want to mess with OpenFlow rules.  I'm guessing that's
a fair share of DPDK port users.

Best regards, Ilya Maximets.
Simon Horman April 6, 2023, 12:48 p.m. UTC | #6
On Thu, Apr 06, 2023 at 02:13:00PM +0200, Ilya Maximets wrote:
> On 4/6/23 08:38, Simon Horman wrote:
> > On Wed, Apr 05, 2023 at 02:07:42PM +0000, lin huang wrote:
> >> Hi Simon,
> >>
> >> : ) 
> >> Thanks for your reply.
> >>
> >> There are some explanations about your comments.
> >>
> >> Best regards, Huang Lin.
> >>
> >>> -----Original Message-----
> >>> From: Simon Horman <simon.horman@corigine.com>
> >>> Sent: Wednesday, April 5, 2023 8:28 PM
> >>> To: lin huang <miterv@outlook.com>
> >>> Cc: dev@openvswitch.org; i.maximets@ovn.org
> >>> Subject: Re: [ovs-dev] [PATCH v1 0/2] Add netdev-dpdk support for ingress and
> >>> egress pkts policer.
> >>>
> >>> On Tue, Apr 04, 2023 at 01:46:05PM +0000, lin huang wrote:
> >>>> Hi Ilya,
> >>>>
> >>>> Pls review my code.
> >>>>
> >>>> I want to add a new policer which is pps-based in netdev-dpdk module.
> >>>> The policer is divided into ingress and egress part. Both use the ovs native
> >>> tocken bucket library as the counter.
> >>>> Compared to bandwidth-based policers, the pps-based policer is more
> >>> effective at combating low-and-slow types of DDoS attacks.
> >>>>
> >>>> This new ingress policer is configured using the "policer_kpkts_rate" and
> >>> "policer_kpkts_burst" configuration statement. Here is the example.
> >>>> ovs-vsctl set interface dpdk0 policer_kpkts_rate=2
> >>>> ovs-vsctl set interface dpdk0 policer_kpkts_burst=2
> >>>>
> >>>> This new egress policer is configured using the " egress-pkts-policer "
> >>> configuration statement. Here is the example.
> >>>> ovs-vsctl set port vhost-user0 qos=@newqos -- \
> >>>>     --id=@newqos create qos type=egress-pkts-policer
> >>> other-config:pkts_rate=2048 \
> >>>>     other-config:pkts_burst=2048`
> >>>
> >>> I have some high level questions about this.
> >>>
> >>> 1. For ingress there is, as you mention above,
> >>>    already options at the ovs-vsctl for port-based PPS policing.
> >>>
> >>>    Did you consider making the egress options you are adding consistent
> >>>    with the existing ingress options?
> >>>    f.e.  egress_policing_kpkts_rate and egress_policing_kpkts_rate.
> >> 	
> >> 	Yes we can add new egress options in interface. But this work will conflict the 
> >>     userspace datapath egress QoS (egress policing) framework. 
> >> 	The framework needs to set up a QoS instance which is configured by cir/cbs/pps... to a specific port.
> >> 	So, I add a new egress policer called pkts-policer.
> > 
> > My point was mainly that for ingress we use 'kpkts'
> > But your proposal for egress uses 'pkts'.
> > I think they should be the same.
> 
> I had the same thought, so pointed that out in my review for the patch.
> I think, we agreed to change to kilo-. The math will also become less
> confusing.

Thanks

> >>> 2. Metering operates on egress, is a policer, and can support PPS.
> >>>    Did you consider using this instead of adding a new egress policer?
> >>
> >> 	In my opinion, meter table is used in flow rate limit not in interface traffic.
> >>     Interface policer is different from meter policer which you can police a specific flow.
> >> 	The new egress policer is just in userspace datapath not in kernel datapath.
> >> 	Does the new policer involve the kernel datapath?
> >>
> >>>
> >>>    There is an upstreamed implementation of this for upstreamed
> >>>    for the kernel datapath with and without TC HW offload.
> > 
> > My point is that, in my experience, metering covers your use-case as I
> > understand it. I'd be interested to hear of an example where it does not.
> 
> I suppose, it's useful for those who are using OVS as a simple learning
> switch and don't want to mess with OpenFlow rules.  I'm guessing that's
> a fair share of DPDK port users.

Point taken.

I guess for my use-cases the complexity of meters was managed at a higher
layer, which made it a good choice - partly because it was already
supported by the higher layers.
miter April 6, 2023, 1:37 p.m. UTC | #7
> -----Original Message-----
> From: Simon Horman <simon.horman@corigine.com>
> Sent: Thursday, April 6, 2023 8:48 PM
> To: Ilya Maximets <i.maximets@ovn.org>
> Cc: lin huang <miterv@outlook.com>; dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH v1 0/2] Add netdev-dpdk support for ingress
> and egress pkts policer.
> 
> On Thu, Apr 06, 2023 at 02:13:00PM +0200, Ilya Maximets wrote:
> > On 4/6/23 08:38, Simon Horman wrote:
> > > On Wed, Apr 05, 2023 at 02:07:42PM +0000, lin huang wrote:
> > >> Hi Simon,
> > >>
> > >> : )
> > >> Thanks for your reply.
> > >>
> > >> There are some explanations about your comments.
> > >>
> > >> Best regards, Huang Lin.
> > >>
> > >>> -----Original Message-----
> > >>> From: Simon Horman <simon.horman@corigine.com>
> > >>> Sent: Wednesday, April 5, 2023 8:28 PM
> > >>> To: lin huang <miterv@outlook.com>
> > >>> Cc: dev@openvswitch.org; i.maximets@ovn.org
> > >>> Subject: Re: [ovs-dev] [PATCH v1 0/2] Add netdev-dpdk support for
> ingress and
> > >>> egress pkts policer.
> > >>>
> > >>> On Tue, Apr 04, 2023 at 01:46:05PM +0000, lin huang wrote:
> > >>>> Hi Ilya,
> > >>>>
> > >>>> Pls review my code.
> > >>>>
> > >>>> I want to add a new policer which is pps-based in netdev-dpdk
> module.
> > >>>> The policer is divided into ingress and egress part. Both use the ovs
> native
> > >>> tocken bucket library as the counter.
> > >>>> Compared to bandwidth-based policers, the pps-based policer is more
> > >>> effective at combating low-and-slow types of DDoS attacks.
> > >>>>
> > >>>> This new ingress policer is configured using the "policer_kpkts_rate"
> and
> > >>> "policer_kpkts_burst" configuration statement. Here is the example.
> > >>>> ovs-vsctl set interface dpdk0 policer_kpkts_rate=2
> > >>>> ovs-vsctl set interface dpdk0 policer_kpkts_burst=2
> > >>>>
> > >>>> This new egress policer is configured using the " egress-pkts-policer "
> > >>> configuration statement. Here is the example.
> > >>>> ovs-vsctl set port vhost-user0 qos=@newqos -- \
> > >>>>     --id=@newqos create qos type=egress-pkts-policer
> > >>> other-config:pkts_rate=2048 \
> > >>>>     other-config:pkts_burst=2048`
> > >>>
> > >>> I have some high level questions about this.
> > >>>
> > >>> 1. For ingress there is, as you mention above,
> > >>>    already options at the ovs-vsctl for port-based PPS policing.
> > >>>
> > >>>    Did you consider making the egress options you are adding consistent
> > >>>    with the existing ingress options?
> > >>>    f.e.  egress_policing_kpkts_rate and egress_policing_kpkts_rate.
> > >>
> > >> 	Yes we can add new egress options in interface. But this work will
> conflict the
> > >>     userspace datapath egress QoS (egress policing) framework.
> > >> 	The framework needs to set up a QoS instance which is configured by
> cir/cbs/pps... to a specific port.
> > >> 	So, I add a new egress policer called pkts-policer.
> > >
> > > My point was mainly that for ingress we use 'kpkts'
> > > But your proposal for egress uses 'pkts'.
> > > I think they should be the same.
> >
> > I had the same thought, so pointed that out in my review for the patch.
> > I think, we agreed to change to kilo-. The math will also become less
> > confusing.
> 
> Thanks

Thanks for your reply.
As Ilya said, we agreed to change egress parameters to kilo.

> 
> > >>> 2. Metering operates on egress, is a policer, and can support PPS.
> > >>>    Did you consider using this instead of adding a new egress policer?
> > >>
> > >> 	In my opinion, meter table is used in flow rate limit not in interface
> traffic.
> > >>     Interface policer is different from meter policer which you can police a
> specific flow.
> > >> 	The new egress policer is just in userspace datapath not in kernel
> datapath.
> > >> 	Does the new policer involve the kernel datapath?
> > >>
> > >>>
> > >>>    There is an upstreamed implementation of this for upstreamed
> > >>>    for the kernel datapath with and without TC HW offload.
> > >
> > > My point is that, in my experience, metering covers your use-case as I
> > > understand it. I'd be interested to hear of an example where it does not.
> >
> > I suppose, it's useful for those who are using OVS as a simple learning
> > switch and don't want to mess with OpenFlow rules.  I'm guessing that's
> > a fair share of DPDK port users.
> 
> Point taken.
> 
> I guess for my use-cases the complexity of meters was managed at a higher
> layer, which made it a good choice - partly because it was already
> supported by the higher layers.
> 

Thank you for sharing your advice with me.

Yes, meters can do kpkts and pps rate limit by openflow rules which can cover my use-cases.
But In some specific case, user just want to limit flow by a simple command line configuration not Openflow rules.
A lot of OpenFlow rules which added by controller will spend a higher memory and cpu cycles.