On Tue, Jul 7, 2020 at 12:14 AM William Tu <[email protected]> wrote: > > On Sun, Jul 5, 2020 at 9:49 PM Tonghao Zhang <[email protected]> wrote: > > > > On Sun, Jul 5, 2020 at 5:34 AM William Tu <[email protected]> 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 > > > > > > Or no action also infers drop > > > $ 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)' '' > > > > > > $ 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 <[email protected]> > > > > > > --- > > > v2: > > > travis: > > > https://travis-ci.org/github/williamtu/ovs-travis/builds/704996060 > > > address feedback from Simon and Tonghao > > > --- > > > lib/netdev-offload-tc.c | 11 +++++++++++ > > > 1 file changed, 11 insertions(+) > > > > > > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c > > > index 258d31f54b08..ce90425c5f1b 100644 > > > --- a/lib/netdev-offload-tc.c > > > +++ b/lib/netdev-offload-tc.c > > > @@ -1788,6 +1788,10 @@ netdev_tc_flow_put(struct netdev *netdev, struct > > > match *match, > > > action->chain = nl_attr_get_u32(nla); > > > flower.action_count++; > > > recirc_act = true; > > > + } 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++; > > > } else { > > > VLOG_DBG_RL(&rl, "unsupported put action type: %d", > > > nl_attr_type(nla)); > > > @@ -1795,6 +1799,13 @@ netdev_tc_flow_put(struct netdev *netdev, struct > > > match *match, > > > } > > > } > > > > > > + if (flower.action_count == 0) { > > > + action = &flower.actions[flower.action_count]; > > > + action->type = TC_ACT_GOTO; > > > + action->chain = 0; /* Drop Action. */ > > > + flower.action_count++; > > > + } > > Sorry for not being clear. > > I mean that we added drop action when there is no action. the call tree is: > > netdev_tc_flow_put > > tc_replace_flower > > nl_msg_put_flower_options > > nl_msg_put_flower_acts: > > if (!flower->action_count) { > > nl_msg_put_act_gact(request, 0); > > .... > > } > > > > And we can also add the check there as your patch did, for rules when no > > action. > Do you mean add the check at both functions ? (nl_msg_put_flower_acts > and netdev_tc_flow_put) > > I'm confused. What's the difference between adding at drop at > nl_msg_put_flower_acts(), as your above code, > and adding the drop at > netdev_tc_flow_put(), as my patch? > > Aren't both handling the case where there is no action, and insert a > drop action? > And we should only need it in one place, either at nl_msg_put_flower_acts or > at > netdev_tc_flow_put. Hi William, yes, we should check it either at nl_msg_put_flower_acts or at netdev_tc_flow_put. nl_msg_put_flower_acts has added check(lib/tc.c 2635 line) for the case where there is no action. It is unnecessary to add check it at netdev_tc_flow_put. and if we add the check at netdev_tc_flow_put. so we can remove check and return err code at nl_msg_put_flower_acts. sorry for not being clear. > William
-- Best regards, Tonghao _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
