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 |
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 >
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
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.
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
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
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 --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));
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(+)