diff mbox series

[ovs-dev] netdev-offload-tc: Add drop action support.

Message ID 1593531045-112153-1-git-send-email-u9012063@gmail.com
State Superseded
Headers show
Series [ovs-dev] netdev-offload-tc: Add drop action support. | expand

Commit Message

William Tu June 30, 2020, 3:30 p.m. UTC 
Currently drop action is not offloaded when using userspace datapath
with tc offload.  The patch programs tc gact (generic action) chain
ID 0 to drop the packet by setting it to TC_ACT_SHOT.

Example:
$ ovs-appctl dpctl/add-flow netdev@ovs-netdev \
  'recirc_id(0),in_port(2),eth(),eth_type(0x0806),\
  arp(op=2,tha=00:50:56:e1:4b:ab,tip=10.255.1.116)' drop

$ tc filter show dev ovs-p0 ingress
filter protocol arp pref 2 flower chain 0
filter protocol arp pref 2 flower chain 0 handle 0x1
  eth_type arp
  arp_tip 10.255.1.116
  arp_op reply
  arp_tha 00:50:56:e1:4b:ab
  skip_hw
  not_in_hw
	action order 1: gact action drop
    ...

Signed-off-by: William Tu <u9012063@gmail.com>
---
 lib/netdev-offload-tc.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Simon Horman June 30, 2020, 4:01 p.m. UTC | #1 
On Tue, Jun 30, 2020 at 08:30:45AM -0700, William Tu wrote:
> Currently drop action is not offloaded when using userspace datapath
> with tc offload.  The patch programs tc gact (generic action) chain
> ID 0 to drop the packet by setting it to TC_ACT_SHOT.
> 
> Example:
> $ ovs-appctl dpctl/add-flow netdev@ovs-netdev \
>   'recirc_id(0),in_port(2),eth(),eth_type(0x0806),\
>   arp(op=2,tha=00:50:56:e1:4b:ab,tip=10.255.1.116)' drop
> 
> $ tc filter show dev ovs-p0 ingress
> filter protocol arp pref 2 flower chain 0
> filter protocol arp pref 2 flower chain 0 handle 0x1
>   eth_type arp
>   arp_tip 10.255.1.116
>   arp_op reply
>   arp_tha 00:50:56:e1:4b:ab
>   skip_hw
>   not_in_hw
> 	action order 1: gact action drop
>     ...
> 
> Signed-off-by: William Tu <u9012063@gmail.com>

Hi William,

this change looks good to me other than that I'm unsure
why we need #ifndef __KERNEL__.

> ---
>  lib/netdev-offload-tc.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 258d31f54b08..8c6bcc0aa2c7 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -1788,6 +1788,12 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>              action->chain = nl_attr_get_u32(nla);
>              flower.action_count++;
>              recirc_act = true;
> +#ifndef __KERNEL__
> +        } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_DROP) {
> +            action->type = TC_ACT_GOTO;
> +            action->chain = 0;  /* 0 is reserved and not used by recirc. */
> +            flower.action_count++;
> +#endif
>          } else {
>              VLOG_DBG_RL(&rl, "unsupported put action type: %d",
>                          nl_attr_type(nla));
> -- 
> 2.7.4
>
William Tu June 30, 2020, 4:06 p.m. UTC | #2 
On Tue, Jun 30, 2020 at 9:01 AM Simon Horman <simon.horman@netronome.com> wrote:
>
> On Tue, Jun 30, 2020 at 08:30:45AM -0700, William Tu wrote:
> > Currently drop action is not offloaded when using userspace datapath
> > with tc offload.  The patch programs tc gact (generic action) chain
> > ID 0 to drop the packet by setting it to TC_ACT_SHOT.
> >
> > Example:
> > $ ovs-appctl dpctl/add-flow netdev@ovs-netdev \
> >   'recirc_id(0),in_port(2),eth(),eth_type(0x0806),\
> >   arp(op=2,tha=00:50:56:e1:4b:ab,tip=10.255.1.116)' drop
> >
> > $ tc filter show dev ovs-p0 ingress
> > filter protocol arp pref 2 flower chain 0
> > filter protocol arp pref 2 flower chain 0 handle 0x1
> >   eth_type arp
> >   arp_tip 10.255.1.116
> >   arp_op reply
> >   arp_tha 00:50:56:e1:4b:ab
> >   skip_hw
> >   not_in_hw
> >       action order 1: gact action drop
> >     ...
> >
> > Signed-off-by: William Tu <u9012063@gmail.com>
>
> Hi William,
>
> this change looks good to me other than that I'm unsure
> why we need #ifndef __KERNEL__.
>
Because for kernel datapath, we don't define
OVS_ACTION_ATTR_DROP

at datapath/linux/compat/include/linux/openvswitch.h
...
#ifndef __KERNEL__
    OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/
    OVS_ACTION_ATTR_TUNNEL_POP,    /* u32 port number. */
    OVS_ACTION_ATTR_DROP,          /* u32 xlate_error. */
    OVS_ACTION_ATTR_LB_OUTPUT,     /* u32 bond-id. */
#endif

Regards,
William
Simon Horman June 30, 2020, 4:13 p.m. UTC | #3 
On Tue, Jun 30, 2020 at 09:06:40AM -0700, William Tu wrote:
> On Tue, Jun 30, 2020 at 9:01 AM Simon Horman <simon.horman@netronome.com> wrote:
> >
> > On Tue, Jun 30, 2020 at 08:30:45AM -0700, William Tu wrote:
> > > Currently drop action is not offloaded when using userspace datapath
> > > with tc offload.  The patch programs tc gact (generic action) chain
> > > ID 0 to drop the packet by setting it to TC_ACT_SHOT.
> > >
> > > Example:
> > > $ ovs-appctl dpctl/add-flow netdev@ovs-netdev \
> > >   'recirc_id(0),in_port(2),eth(),eth_type(0x0806),\
> > >   arp(op=2,tha=00:50:56:e1:4b:ab,tip=10.255.1.116)' drop
> > >
> > > $ tc filter show dev ovs-p0 ingress
> > > filter protocol arp pref 2 flower chain 0
> > > filter protocol arp pref 2 flower chain 0 handle 0x1
> > >   eth_type arp
> > >   arp_tip 10.255.1.116
> > >   arp_op reply
> > >   arp_tha 00:50:56:e1:4b:ab
> > >   skip_hw
> > >   not_in_hw
> > >       action order 1: gact action drop
> > >     ...
> > >
> > > Signed-off-by: William Tu <u9012063@gmail.com>
> >
> > Hi William,
> >
> > this change looks good to me other than that I'm unsure
> > why we need #ifndef __KERNEL__.
> >
> Because for kernel datapath, we don't define
> OVS_ACTION_ATTR_DROP
> 
> at datapath/linux/compat/include/linux/openvswitch.h
> ...
> #ifndef __KERNEL__
>     OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/
>     OVS_ACTION_ATTR_TUNNEL_POP,    /* u32 port number. */
>     OVS_ACTION_ATTR_DROP,          /* u32 xlate_error. */
>     OVS_ACTION_ATTR_LB_OUTPUT,     /* u32 bond-id. */
> #endif

I feel that I'm missing something terribly obvious, but it seems to me that
lib/netdev-offload-tc.c is only ever compiled as user-space code (its not
compiled into the kernel datapath) and thus __KERNEL__ would never be set.
William Tu June 30, 2020, 4:29 p.m. UTC | #4 
On Tue, Jun 30, 2020 at 9:13 AM Simon Horman <simon.horman@netronome.com> wrote:
>
> On Tue, Jun 30, 2020 at 09:06:40AM -0700, William Tu wrote:
> > On Tue, Jun 30, 2020 at 9:01 AM Simon Horman <simon.horman@netronome.com> wrote:
> > >
> > > On Tue, Jun 30, 2020 at 08:30:45AM -0700, William Tu wrote:
> > > > Currently drop action is not offloaded when using userspace datapath
> > > > with tc offload.  The patch programs tc gact (generic action) chain
> > > > ID 0 to drop the packet by setting it to TC_ACT_SHOT.
> > > >
> > > > Example:
> > > > $ ovs-appctl dpctl/add-flow netdev@ovs-netdev \
> > > >   'recirc_id(0),in_port(2),eth(),eth_type(0x0806),\
> > > >   arp(op=2,tha=00:50:56:e1:4b:ab,tip=10.255.1.116)' drop
> > > >
> > > > $ tc filter show dev ovs-p0 ingress
> > > > filter protocol arp pref 2 flower chain 0
> > > > filter protocol arp pref 2 flower chain 0 handle 0x1
> > > >   eth_type arp
> > > >   arp_tip 10.255.1.116
> > > >   arp_op reply
> > > >   arp_tha 00:50:56:e1:4b:ab
> > > >   skip_hw
> > > >   not_in_hw
> > > >       action order 1: gact action drop
> > > >     ...
> > > >
> > > > Signed-off-by: William Tu <u9012063@gmail.com>
> > >
> > > Hi William,
> > >
> > > this change looks good to me other than that I'm unsure
> > > why we need #ifndef __KERNEL__.
> > >
> > Because for kernel datapath, we don't define
> > OVS_ACTION_ATTR_DROP
> >
> > at datapath/linux/compat/include/linux/openvswitch.h
> > ...
> > #ifndef __KERNEL__
> >     OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/
> >     OVS_ACTION_ATTR_TUNNEL_POP,    /* u32 port number. */
> >     OVS_ACTION_ATTR_DROP,          /* u32 xlate_error. */
> >     OVS_ACTION_ATTR_LB_OUTPUT,     /* u32 bond-id. */
> > #endif
>
> I feel that I'm missing something terribly obvious, but it seems to me that
> lib/netdev-offload-tc.c is only ever compiled as user-space code (its not
> compiled into the kernel datapath) and thus __KERNEL__ would never be set.

I see, so probably checking __KERNEL__ here is not necessary. Will remove it.

William
Tonghao Zhang July 2, 2020, 11:24 a.m. UTC | #5 
On Wed, Jul 1, 2020 at 12:30 AM William Tu <u9012063@gmail.com> wrote:
>
> On Tue, Jun 30, 2020 at 9:13 AM Simon Horman <simon.horman@netronome.com> wrote:
> >
> > On Tue, Jun 30, 2020 at 09:06:40AM -0700, William Tu wrote:
> > > On Tue, Jun 30, 2020 at 9:01 AM Simon Horman <simon.horman@netronome.com> wrote:
> > > >
> > > > On Tue, Jun 30, 2020 at 08:30:45AM -0700, William Tu wrote:
> > > > > Currently drop action is not offloaded when using userspace datapath
> > > > > with tc offload.  The patch programs tc gact (generic action) chain
> > > > > ID 0 to drop the packet by setting it to TC_ACT_SHOT.
> > > > >
> > > > > Example:
> > > > > $ ovs-appctl dpctl/add-flow netdev@ovs-netdev \
> > > > >   'recirc_id(0),in_port(2),eth(),eth_type(0x0806),\
> > > > >   arp(op=2,tha=00:50:56:e1:4b:ab,tip=10.255.1.116)' drop
> > > > >
> > > > > $ tc filter show dev ovs-p0 ingress
> > > > > filter protocol arp pref 2 flower chain 0
> > > > > filter protocol arp pref 2 flower chain 0 handle 0x1
> > > > >   eth_type arp
> > > > >   arp_tip 10.255.1.116
> > > > >   arp_op reply
> > > > >   arp_tha 00:50:56:e1:4b:ab
> > > > >   skip_hw
> > > > >   not_in_hw
> > > > >       action order 1: gact action drop
> > > > >     ...
> > > > >
> > > > > Signed-off-by: William Tu <u9012063@gmail.com>
> > > >
> > > > Hi William,
> > > >
> > > > this change looks good to me other than that I'm unsure
> > > > why we need #ifndef __KERNEL__.
> > > >
> > > Because for kernel datapath, we don't define
> > > OVS_ACTION_ATTR_DROP
> > >
> > > at datapath/linux/compat/include/linux/openvswitch.h
> > > ...
> > > #ifndef __KERNEL__
> > >     OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/
> > >     OVS_ACTION_ATTR_TUNNEL_POP,    /* u32 port number. */
> > >     OVS_ACTION_ATTR_DROP,          /* u32 xlate_error. */
> > >     OVS_ACTION_ATTR_LB_OUTPUT,     /* u32 bond-id. */
> > > #endif
> >
> > I feel that I'm missing something terribly obvious, but it seems to me that
> > lib/netdev-offload-tc.c is only ever compiled as user-space code (its not
> > compiled into the kernel datapath) and thus __KERNEL__ would never be set.
>
> I see, so probably checking __KERNEL__ here is not necessary. Will remove it.
Hi William
It looks good to me.
BTW, If there is no action, we use the drop action as default.
$ ovs-appctl dpctl/add-flow
'recirc_id(0),in_port(3),eth(),eth_type(0x0806),arp(op=2,tha=00:50:56:e1:4b:ab,tip=10.255.1.116)'
""
> William




--
Best regards, Tonghao
William Tu July 4, 2020, 8:33 p.m. UTC | #6 
On Thu, Jul 02, 2020 at 07:24:35PM +0800, Tonghao Zhang wrote:
> On Wed, Jul 1, 2020 at 12:30 AM William Tu <u9012063@gmail.com> wrote:
> >
> > On Tue, Jun 30, 2020 at 9:13 AM Simon Horman <simon.horman@netronome.com> wrote:
> > >
> > > On Tue, Jun 30, 2020 at 09:06:40AM -0700, William Tu wrote:
> > > > On Tue, Jun 30, 2020 at 9:01 AM Simon Horman <simon.horman@netronome.com> wrote:
> > > > >
> > > > > On Tue, Jun 30, 2020 at 08:30:45AM -0700, William Tu wrote:
> > > > > > Currently drop action is not offloaded when using userspace datapath
> > > > > > with tc offload.  The patch programs tc gact (generic action) chain
> > > > > > ID 0 to drop the packet by setting it to TC_ACT_SHOT.
> > > > > >
> > > > > > Example:
> > > > > > $ ovs-appctl dpctl/add-flow netdev@ovs-netdev \
> > > > > >   'recirc_id(0),in_port(2),eth(),eth_type(0x0806),\
> > > > > >   arp(op=2,tha=00:50:56:e1:4b:ab,tip=10.255.1.116)' drop
> > > > > >
> > > > > > $ tc filter show dev ovs-p0 ingress
> > > > > > filter protocol arp pref 2 flower chain 0
> > > > > > filter protocol arp pref 2 flower chain 0 handle 0x1
> > > > > >   eth_type arp
> > > > > >   arp_tip 10.255.1.116
> > > > > >   arp_op reply
> > > > > >   arp_tha 00:50:56:e1:4b:ab
> > > > > >   skip_hw
> > > > > >   not_in_hw
> > > > > >       action order 1: gact action drop
> > > > > >     ...
> > > > > >
> > > > > > Signed-off-by: William Tu <u9012063@gmail.com>
> > > > >
> > > > > Hi William,
> > > > >
> > > > > this change looks good to me other than that I'm unsure
> > > > > why we need #ifndef __KERNEL__.
> > > > >
> > > > Because for kernel datapath, we don't define
> > > > OVS_ACTION_ATTR_DROP
> > > >
> > > > at datapath/linux/compat/include/linux/openvswitch.h
> > > > ...
> > > > #ifndef __KERNEL__
> > > >     OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/
> > > >     OVS_ACTION_ATTR_TUNNEL_POP,    /* u32 port number. */
> > > >     OVS_ACTION_ATTR_DROP,          /* u32 xlate_error. */
> > > >     OVS_ACTION_ATTR_LB_OUTPUT,     /* u32 bond-id. */
> > > > #endif
> > >
> > > I feel that I'm missing something terribly obvious, but it seems to me that
> > > lib/netdev-offload-tc.c is only ever compiled as user-space code (its not
> > > compiled into the kernel datapath) and thus __KERNEL__ would never be set.
> >
> > I see, so probably checking __KERNEL__ here is not necessary. Will remove it.
> Hi William
> It looks good to me.
> BTW, If there is no action, we use the drop action as default.
> $ ovs-appctl dpctl/add-flow
> 'recirc_id(0),in_port(3),eth(),eth_type(0x0806),arp(op=2,tha=00:50:56:e1:4b:ab,tip=10.255.1.116)'

Thanks, I will fix it in v2.
William
diff mbox series

Patch

diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 258d31f54b08..8c6bcc0aa2c7 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -1788,6 +1788,12 @@  netdev_tc_flow_put(struct netdev *netdev, struct match *match,
             action->chain = nl_attr_get_u32(nla);
             flower.action_count++;
             recirc_act = true;
+#ifndef __KERNEL__
+        } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_DROP) {
+            action->type = TC_ACT_GOTO;
+            action->chain = 0;  /* 0 is reserved and not used by recirc. */
+            flower.action_count++;
+#endif
         } else {
             VLOG_DBG_RL(&rl, "unsupported put action type: %d",
                         nl_attr_type(nla));