| Message ID | 20191203134534.32056-8-roid@mellanox.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for offloading CT datapath rules to TC | expand |
On 03.12.2019 14:45, Roi Dayan wrote: > From: Paul Blakey <paulb@mellanox.com> > > Each recirculation id will create a tc chain, and we translate > the recirculation action to a tc goto chain action. > > We check for kernel support for this by probing OvS Datapath for the > tc recirc id sharing feature. If supported, we can offload rules > that match on recirc_id, and recirculation action safely. > --- > > Changelog: > V2->V3: > Merged part of probe for recirc_id support in here to help future git bisect. > Added tunnel released check to avoid bug with mirroring > Removed cascading condition in netdev_tc_flow_put() check of recirc_id support > > V1->V2: > moved make_tc_id_chain helper to tc.h as static inline > updated is_tc_id_eq with chain compare instead of find_ufid > > Signed-off-by: Paul Blakey <paulb@mellanox.com> This tag should go before the '---', otherwise it'll not be part of a commit-message. You may see that checkpatch complains about missing signed-off on some of the patches. > --- > lib/dpif-netlink.c | 1 + > lib/netdev-offload-tc.c | 61 +++++++++++++++++++++++++++++++++++++++++-------- > lib/tc.c | 49 ++++++++++++++++++++++++++++++++++----- > lib/tc.h | 18 ++++++++++++++- > 4 files changed, 112 insertions(+), 17 deletions(-) > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c > index 92da918544d1..f0e870543ae5 100644 > --- a/lib/dpif-netlink.c > +++ b/lib/dpif-netlink.c > @@ -1638,6 +1638,7 @@ dpif_netlink_netdev_match_to_dpif_flow(struct match *match, > .mask = &match->wc.masks, > .support = { > .max_vlan_headers = 2, > + .recirc = true, > }, > }; > size_t offset; > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c > index bb0eccd79ceb..7af5ad683bb7 100644 > --- a/lib/netdev-offload-tc.c > +++ b/lib/netdev-offload-tc.c > @@ -38,6 +38,7 @@ > #include "tc.h" > #include "unaligned.h" > #include "util.h" > +#include "dpif-provider.h" > > VLOG_DEFINE_THIS_MODULE(netdev_offload_tc); > > @@ -206,9 +207,12 @@ static void > add_ufid_tc_mapping(struct netdev *netdev, const ovs_u128 *ufid, > struct tcf_id *id) > { > - size_t ufid_hash = hash_bytes(ufid, sizeof *ufid, 0); > - size_t tc_hash = hash_int(hash_int(id->prio, id->handle), id->ifindex); > struct ufid_tc_data *new_data = xzalloc(sizeof *new_data); > + size_t ufid_hash = hash_bytes(ufid, sizeof *ufid, 0); > + size_t tc_hash; > + > + tc_hash = hash_int(hash_int(id->prio, id->handle), id->ifindex); > + tc_hash = hash_int(id->chain, tc_hash); > > new_data->ufid = *ufid; > new_data->id = *id; > @@ -252,8 +256,11 @@ get_ufid_tc_mapping(const ovs_u128 *ufid, struct tcf_id *id) > static bool > find_ufid(struct netdev *netdev, struct tcf_id *id, ovs_u128 *ufid) > { > - size_t tc_hash = hash_int(hash_int(id->prio, id->handle), id->ifindex); > struct ufid_tc_data *data; > + size_t tc_hash; > + > + tc_hash = hash_int(hash_int(id->prio, id->handle), id->ifindex); > + tc_hash = hash_int(id->chain, tc_hash); > > ovs_mutex_lock(&ufid_lock); > HMAP_FOR_EACH_WITH_HASH (data, tc_to_ufid_node, tc_hash, &tc_to_ufid) { > @@ -739,6 +746,10 @@ parse_tc_flower_to_match(struct tc_flower *flower, > nl_msg_put_u32(buf, OVS_ACTION_ATTR_OUTPUT, odp_to_u32(outport)); > } > break; > + case TC_ACT_GOTO: { > + nl_msg_put_u32(buf, OVS_ACTION_ATTR_RECIRC, action->chain); > + } > + break; > } > } > } > @@ -799,6 +810,7 @@ netdev_tc_flow_dump_next(struct netdev_flow_dump *dump, > > match->wc.masks.in_port.odp_port = u32_to_odp(UINT32_MAX); > match->flow.in_port.odp_port = dump->port; > + match_set_recirc_id(match, id.chain); > > return true; > } > @@ -983,12 +995,6 @@ test_key_and_mask(struct match *match) > return EOPNOTSUPP; > } > > - if (mask->recirc_id && key->recirc_id) { > - VLOG_DBG_RL(&rl, "offloading attribute recirc_id isn't supported"); > - return EOPNOTSUPP; > - } > - mask->recirc_id = 0; > - > if (mask->dp_hash) { > VLOG_DBG_RL(&rl, "offloading attribute dp_hash isn't supported"); > return EOPNOTSUPP; > @@ -1139,6 +1145,25 @@ flower_match_to_tun_opt(struct tc_flower *flower, const struct flow_tnl *tnl, > flower->mask.tunnel.metadata.present.len = tnl->metadata.present.len; > } > > +static bool > +recirc_id_sharing_support(struct dpif *dpif) > +{ > + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; > + static bool supported = false; > + int err; > + > + if (ovsthread_once_start(&once)) { > + err = dpif_set_features(dpif, OVS_DP_F_TC_RECIRC_SHARING); I don't think that it's the right thing to do to change datapath configuration from the netdev-offload implementation. This also requires you to have access to the dpif-provider internals breaking the OVS architecture of modules. (You may see that this patch set doesn't build at some point because you need to know the internal structure of a dpif instance.) I'd suggest to move this to the common feature probing code, i.e. to ofproto. After that you may pass supported features via offload_info structure. Thoughts? Best regards, Ilya Maximets.
Hi sorry for late reply, didn't get this email. On 03.12.2019 16:16, Ilya Maximets wrote: > From: Ilya Maximets <i.maximets@samsung.com> > On 03.12.2019 14:45, Roi Dayan wrote: > > From: Paul Blakey <paulb@mellanox.com> > > > > Each recirculation id will create a tc chain, and we translate > > the recirculation action to a tc goto chain action. > > > > We check for kernel support for this by probing OvS Datapath for the > > tc recirc id sharing feature. If supported, we can offload rules > > that match on recirc_id, and recirculation action safely. > > --- > > > > Changelog: > > V2->V3: > > Merged part of probe for recirc_id support in here to help future git > > bisect. > > Added tunnel released check to avoid bug with mirroring > > Removed cascading condition in netdev_tc_flow_put() check of recirc_id > > support > > > > V1->V2: > > moved make_tc_id_chain helper to tc.h as static inline > > updated is_tc_id_eq with chain compare instead of find_ufid > > > > Signed-off-by: Paul Blakey <pa...@mellanox.com> > > This tag should go before the '---', otherwise it'll not be part of a > commit-message. > You may see that checkpatch complains about missing signed-off on some of the > patches. Yea we say it, but since I wanted to remove this changelog for last revision it was just easier to manage than git notes :) will do it for next one. > > --- > > lib/dpif-netlink.c | 1 + > > lib/netdev-offload-tc.c | 61 > > +++++++++++++++++++++++++++++++++++++++++-------- > > lib/tc.c | 49 ++++++++++++++++++++++++++++++++++----- > > lib/tc.h | 18 ++++++++++++++- > > 4 files changed, 112 insertions(+), 17 deletions(-) > > > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c > > index 92da918544d1..f0e870543ae5 100644 > > --- a/lib/dpif-netlink.c > > +++ b/lib/dpif-netlink.c [....] > > @@ -983,12 +995,6 @@ test_key_and_mask(struct match *match) > > return EOPNOTSUPP; > > } > > > > - if (mask->recirc_id && key->recirc_id) { > > - VLOG_DBG_RL(&rl, "offloading attribute recirc_id isn't supported"); > > - return EOPNOTSUPP; > > - } > > - mask->recirc_id = 0; > > - > > if (mask->dp_hash) { > > VLOG_DBG_RL(&rl, "offloading attribute dp_hash isn't supported"); > > return EOPNOTSUPP; > > @@ -1139,6 +1145,25 @@ flower_match_to_tun_opt(struct tc_flower *flower, > > const struct flow_tnl *tnl, > > flower->mask.tunnel.metadata.present.len = tnl->metadata.present.len; > > } > > > > +static bool > > +recirc_id_sharing_support(struct dpif *dpif) > > +{ > > + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; > > + static bool supported = false; > > + int err; > > + > > + if (ovsthread_once_start(&once)) { > > + err = dpif_set_features(dpif, OVS_DP_F_TC_RECIRC_SHARING); > > I don't think that it's the right thing to do to change datapath configuration > from the netdev-offload implementation. This also requires you to have access > to the dpif-provider internals breaking the OVS architecture of modules. (You > may see that this patch set doesn't build at some point because you need to know > the internal structure of a dpif instance.) > > I'd suggest to move this to the common feature probing code, i.e. to ofproto. > After that you may pass supported features via offload_info structure. > > Thoughts? > We are calling dpif_set_features, not accessing dpif internals. The thing is, that this sets a feature and not just probes for a feature. It enables a static branch on the kernel side which we might not want to enable any time, and an offloading a recirc() action was our hook to know that this is wanted, as we talked about in the kernel patch. I'm not sure how to do that from ofproto layer.
> Hi sorry for late reply, didn't get this email. > > On 03.12.2019 16:16, Ilya Maximets wrote: > > From: Ilya Maximets <i.maximets at samsung.com> > > On 03.12.2019 14:45, Roi Dayan wrote: > > > From: Paul Blakey <paulb at mellanox.com> > > > > > > Each recirculation id will create a tc chain, and we translate > > > the recirculation action to a tc goto chain action. > > > > > > We check for kernel support for this by probing OvS Datapath for the > > > tc recirc id sharing feature. If supported, we can offload rules > > > that match on recirc_id, and recirculation action safely. > > > --- > > > > > > Changelog: > > > V2->V3: > > > Merged part of probe for recirc_id support in here to help > future git > > > bisect. > > > Added tunnel released check to avoid bug with mirroring > > > Removed cascading condition in netdev_tc_flow_put() check of > recirc_id > > > support > > > > > > V1->V2: > > > moved make_tc_id_chain helper to tc.h as static inline > > > updated is_tc_id_eq with chain compare instead of find_ufid > > > > > > Signed-off-by: Paul Blakey <pa... at mellanox.com> > > > > This tag should go before the '---', otherwise it'll not be part of a > > commit-message. > > You may see that checkpatch complains about missing signed-off on > some of the > > patches. > > Yea we say it, but since I wanted to remove this changelog for last > revision it was just easier to manage than git notes :) will do it for > next one. > > > > --- > > > lib/dpif-netlink.c | 1 + > > > lib/netdev-offload-tc.c | 61 > > > +++++++++++++++++++++++++++++++++++++++++-------- > > > lib/tc.c | 49 ++++++++++++++++++++++++++++++++++----- > > > lib/tc.h | 18 ++++++++++++++- > > > 4 files changed, 112 insertions(+), 17 deletions(-) > > > > > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c > > > index 92da918544d1..f0e870543ae5 100644 > > > --- a/lib/dpif-netlink.c > > > +++ b/lib/dpif-netlink.c > > [....] > > > > @@ -983,12 +995,6 @@ test_key_and_mask(struct match *match) > > > return EOPNOTSUPP; > > > } > > > > > > - if (mask->recirc_id && key->recirc_id) { > > > - VLOG_DBG_RL(&rl, "offloading attribute recirc_id isn't > supported"); > > > - return EOPNOTSUPP; > > > - } > > > - mask->recirc_id = 0; > > > - > > > if (mask->dp_hash) { > > > VLOG_DBG_RL(&rl, "offloading attribute dp_hash isn't > supported"); > > > return EOPNOTSUPP; > > > @@ -1139,6 +1145,25 @@ flower_match_to_tun_opt(struct tc_flower > *flower, > > > const struct flow_tnl *tnl, > > > flower->mask.tunnel.metadata.present.len = > tnl->metadata.present.len; > > > } > > > > > > +static bool > > > +recirc_id_sharing_support(struct dpif *dpif) > > > +{ > > > + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; > > > + static bool supported = false; > > > + int err; > > > + > > > + if (ovsthread_once_start(&once)) { > > > + err = dpif_set_features(dpif, OVS_DP_F_TC_RECIRC_SHARING); > > > > I don't think that it's the right thing to do to change datapath > configuration > > from the netdev-offload implementation. This also requires you to > have access > > to the dpif-provider internals breaking the OVS architecture of > modules. (You > > may see that this patch set doesn't build at some point because you > need to know > > the internal structure of a dpif instance.) > > > > I'd suggest to move this to the common feature probing code, i.e. to > ofproto. > > After that you may pass supported features via offload_info structure. > > > > Thoughts? > > > > We are calling dpif_set_features, not accessing dpif internals. Not here, but you're dereferencing dpif pointer later producing the build failure: lib/netdev-offload-tc.c:1371:64: error: using member 'dpif_class' in incomplete struct dpif > > The thing is, that this sets a feature and not just probes for a feature. > > It enables a static branch on the kernel side which we might not want to > enable any time, Does it have any significant performance impact? Looking at the kernel code I don't see any special heavy operations. > > and an offloading a recirc() action was our hook to know that this is > wanted, as we talked > > about in the kernel patch. I do not know what you've talked about while upstreaming kernel parts, but current patch-set for OVS doesn't work for several reasons: 1. Obviously, it doesn't build successfully, because netdev-offload implementation now requires access to the internals of 'struct dpif'. 2. You're using ovsthread_once to not enable feature twice and this will break CT offloading if you'll remove datapath and create it back. You will not be able to enable feature for the newly created datapath. 3. offloading module doesn't depend on datapath and could be theoretically used from the userspace datapath at the same time and if userspace datapath will reach feature enabling code first it will try to set datapath features, will fail and kernel datapath will never try to enable feature because of the same ovsthread_once. So, taking above issues into account, feature enabling should definitely happen on datapath level and might or might not be controlled by the ofproto. > > I'm not sure how to do that from ofproto layer. We may not involve the ofproto layer. For example, you may try to enable the feature in dpif_netlink_open() and fallback if not supported. While doing that you may remember the status of this feature and then send the status to netdev-offload module via 'struct offload_info' or check directly by implementing dpif_get_[user_]features(). Best regards, Ilya Maximets.
On 12/13/2019 2:05 PM, Ilya Maximets wrote: >> Hi sorry for late reply, didn't get this email. >> >> On 03.12.2019 16:16, Ilya Maximets wrote: >> > From: Ilya Maximets <i.maximets at samsung.com> >> > On 03.12.2019 14:45, Roi Dayan wrote: >> > > From: Paul Blakey <paulb at mellanox.com> >> > > [...] >> >> We are calling dpif_set_features, not accessing dpif internals. > Not here, but you're dereferencing dpif pointer later producing the build > failure: > lib/netdev-offload-tc.c:1371:64: error: using member 'dpif_class' in incomplete struct dpif > >> The thing is, that this sets a feature and not just probes for a feature. >> >> It enables a static branch on the kernel side which we might not want to >> enable any time, > Does it have any significant performance impact? Looking at the kernel > code I don't see any special heavy operations. I don't see any heavy operations as well, it was just suggested by mailing list, We just lookup the skb extension on vport recv. For normal packets this will just incur a simple inline check of skb_ext_find (which just does 'return skb->active_extensions & (1 << id)'), which will return false. > >> and an offloading a recirc() action was our hook to know that this is >> wanted, as we talked >> >> about in the kernel patch. > I do not know what you've talked about while upstreaming kernel parts, > but current patch-set for OVS doesn't work for several reasons: > > 1. Obviously, it doesn't build successfully, because netdev-offload > implementation now requires access to the internals of 'struct dpif'. Right I will check that. > > 2. You're using ovsthread_once to not enable feature twice and this will > break CT offloading if you'll remove datapath and create it back. You > will not be able to enable feature for the newly created datapath. Thanks, will fix that. > > 3. offloading module doesn't depend on datapath and could be theoretically > used from the userspace datapath at the same time and if userspace > datapath will reach feature enabling code first it will try to set > datapath features, will fail and kernel datapath will never try to > enable feature because of the same ovsthread_once. > > So, taking above issues into account, feature enabling should definitely > happen on datapath level and might or might not be controlled by the > ofproto. > >> I'm not sure how to do that from ofproto layer. > We may not involve the ofproto layer. For example, you may try to enable > the feature in dpif_netlink_open() and fallback if not supported. While > doing that you may remember the status of this feature and then send the > status to netdev-offload module via 'struct offload_info' or check directly > by implementing dpif_get_[user_]features(). As the performance impact is very minor, I'm for enabling/querying this feature at the start (probably via dpif_set_features) after a successfully creation in dpif_netlink_open of dpif-netlink, will that be ok with you? Thanks, Paul.
On 15.12.2019 14:35, Paul Blakey wrote: > > On 12/13/2019 2:05 PM, Ilya Maximets wrote: >>> Hi sorry for late reply, didn't get this email. >>> >>> On 03.12.2019 16:16, Ilya Maximets wrote: >>> > From: Ilya Maximets <i.maximets at samsung.com> >>> > On 03.12.2019 14:45, Roi Dayan wrote: >>> > > From: Paul Blakey <paulb at mellanox.com> >>> > > > > [...] > >>> >>> We are calling dpif_set_features, not accessing dpif internals. >> Not here, but you're dereferencing dpif pointer later producing the build >> failure: >> lib/netdev-offload-tc.c:1371:64: error: using member 'dpif_class' in incomplete struct dpif >> >>> The thing is, that this sets a feature and not just probes for a feature. >>> >>> It enables a static branch on the kernel side which we might not want to >>> enable any time, >> Does it have any significant performance impact? Looking at the kernel >> code I don't see any special heavy operations. > > I don't see any heavy operations as well, it was just suggested by > mailing list, > > We just lookup the skb extension on vport recv. For normal packets this > will just incur a simple inline check of skb_ext_find (which just does > 'return skb->active_extensions & (1 << id)'), > > which will return false. > > >> >>> and an offloading a recirc() action was our hook to know that this is >>> wanted, as we talked >>> >>> about in the kernel patch. >> I do not know what you've talked about while upstreaming kernel parts, >> but current patch-set for OVS doesn't work for several reasons: >> >> 1. Obviously, it doesn't build successfully, because netdev-offload >> implementation now requires access to the internals of 'struct dpif'. > Right I will check that. >> >> 2. You're using ovsthread_once to not enable feature twice and this will >> break CT offloading if you'll remove datapath and create it back. You >> will not be able to enable feature for the newly created datapath. > > Thanks, will fix that. > >> >> 3. offloading module doesn't depend on datapath and could be theoretically >> used from the userspace datapath at the same time and if userspace >> datapath will reach feature enabling code first it will try to set >> datapath features, will fail and kernel datapath will never try to >> enable feature because of the same ovsthread_once. >> >> So, taking above issues into account, feature enabling should definitely >> happen on datapath level and might or might not be controlled by the >> ofproto. >> >>> I'm not sure how to do that from ofproto layer. >> We may not involve the ofproto layer. For example, you may try to enable >> the feature in dpif_netlink_open() and fallback if not supported. While >> doing that you may remember the status of this feature and then send the >> status to netdev-offload module via 'struct offload_info' or check directly >> by implementing dpif_get_[user_]features(). > > > As the performance impact is very minor, > > I'm for enabling/querying this feature at the start (probably via > dpif_set_features) after a successfully creation in dpif_netlink_open of > dpif-netlink, > > will that be ok with you? I'm not against enabling by default if it doesn't cause performance degradation. Just be sure that we could fall back if the feature is not supported by kernel. BTW, while offloading it might be better to use 'struct offload_info' to pass the feature support status. This way you'll not need to operate with/dereference dpif pointers. > > Thanks, > > Paul. >
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index 92da918544d1..f0e870543ae5 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -1638,6 +1638,7 @@ dpif_netlink_netdev_match_to_dpif_flow(struct match *match, .mask = &match->wc.masks, .support = { .max_vlan_headers = 2, + .recirc = true, }, }; size_t offset; diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index bb0eccd79ceb..7af5ad683bb7 100644 --- a/lib/netdev-offload-tc.c +++ b/lib/netdev-offload-tc.c @@ -38,6 +38,7 @@ #include "tc.h" #include "unaligned.h" #include "util.h" +#include "dpif-provider.h" VLOG_DEFINE_THIS_MODULE(netdev_offload_tc); @@ -206,9 +207,12 @@ static void add_ufid_tc_mapping(struct netdev *netdev, const ovs_u128 *ufid, struct tcf_id *id) { - size_t ufid_hash = hash_bytes(ufid, sizeof *ufid, 0); - size_t tc_hash = hash_int(hash_int(id->prio, id->handle), id->ifindex); struct ufid_tc_data *new_data = xzalloc(sizeof *new_data); + size_t ufid_hash = hash_bytes(ufid, sizeof *ufid, 0); + size_t tc_hash; + + tc_hash = hash_int(hash_int(id->prio, id->handle), id->ifindex); + tc_hash = hash_int(id->chain, tc_hash); new_data->ufid = *ufid; new_data->id = *id; @@ -252,8 +256,11 @@ get_ufid_tc_mapping(const ovs_u128 *ufid, struct tcf_id *id) static bool find_ufid(struct netdev *netdev, struct tcf_id *id, ovs_u128 *ufid) { - size_t tc_hash = hash_int(hash_int(id->prio, id->handle), id->ifindex); struct ufid_tc_data *data; + size_t tc_hash; + + tc_hash = hash_int(hash_int(id->prio, id->handle), id->ifindex); + tc_hash = hash_int(id->chain, tc_hash); ovs_mutex_lock(&ufid_lock); HMAP_FOR_EACH_WITH_HASH (data, tc_to_ufid_node, tc_hash, &tc_to_ufid) { @@ -739,6 +746,10 @@ parse_tc_flower_to_match(struct tc_flower *flower, nl_msg_put_u32(buf, OVS_ACTION_ATTR_OUTPUT, odp_to_u32(outport)); } break; + case TC_ACT_GOTO: { + nl_msg_put_u32(buf, OVS_ACTION_ATTR_RECIRC, action->chain); + } + break; } } } @@ -799,6 +810,7 @@ netdev_tc_flow_dump_next(struct netdev_flow_dump *dump, match->wc.masks.in_port.odp_port = u32_to_odp(UINT32_MAX); match->flow.in_port.odp_port = dump->port; + match_set_recirc_id(match, id.chain); return true; } @@ -983,12 +995,6 @@ test_key_and_mask(struct match *match) return EOPNOTSUPP; } - if (mask->recirc_id && key->recirc_id) { - VLOG_DBG_RL(&rl, "offloading attribute recirc_id isn't supported"); - return EOPNOTSUPP; - } - mask->recirc_id = 0; - if (mask->dp_hash) { VLOG_DBG_RL(&rl, "offloading attribute dp_hash isn't supported"); return EOPNOTSUPP; @@ -1139,6 +1145,25 @@ flower_match_to_tun_opt(struct tc_flower *flower, const struct flow_tnl *tnl, flower->mask.tunnel.metadata.present.len = tnl->metadata.present.len; } +static bool +recirc_id_sharing_support(struct dpif *dpif) +{ + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; + static bool supported = false; + int err; + + if (ovsthread_once_start(&once)) { + err = dpif_set_features(dpif, OVS_DP_F_TC_RECIRC_SHARING); + supported = err ? false : true; + if (supported) { + VLOG_INFO("probe tc: tc recirc id sharing with OvS datapath is supported."); + } + ovsthread_once_done(&once); + } + + return supported; +} + static int netdev_tc_flow_put(struct netdev *netdev, struct match *match, struct nlattr *actions, size_t actions_len, @@ -1156,6 +1181,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, uint32_t block_id = 0; struct nlattr *nla; struct tcf_id id; + uint32_t chain; size_t left; int prio = 0; int ifindex; @@ -1170,6 +1196,12 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, memset(&flower, 0, sizeof flower); + chain = key->recirc_id; + mask->recirc_id = 0; + if (chain && !recirc_id_sharing_support(info->dpif)) { + return -EOPNOTSUPP; + } + if (flow_tnl_dst_is_set(&key->tunnel)) { VLOG_DBG_RL(&rl, "tunnel: id %#" PRIx64 " src " IP_FMT @@ -1421,6 +1453,14 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, if (err) { return err; } + } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_RECIRC) { + if (!recirc_id_sharing_support(info->dpif)) { + return EOPNOTSUPP; + } + + action->type = TC_ACT_GOTO; + action->chain = nl_attr_get_u32(nla); + flower.action_count++; } else { VLOG_DBG_RL(&rl, "unsupported put action type: %d", nl_attr_type(nla)); @@ -1443,7 +1483,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, flower.act_cookie.data = ufid; flower.act_cookie.len = sizeof *ufid; - id = tc_make_tcf_id(ifindex, block_id, prio, hook); + id = tc_make_tcf_id_chain(ifindex, block_id, chain, prio, hook); err = tc_replace_flower(&id, &flower); if (!err) { if (stats) { @@ -1491,6 +1531,7 @@ netdev_tc_flow_get(struct netdev *netdev, match->wc.masks.in_port.odp_port = u32_to_odp(UINT32_MAX); match->flow.in_port.odp_port = in_port; + match_set_recirc_id(match, id.chain); return 0; } diff --git a/lib/tc.c b/lib/tc.c index bc87d4380846..48d10da7055f 100644 --- a/lib/tc.c +++ b/lib/tc.c @@ -51,6 +51,7 @@ #endif #if TCA_MAX < 14 +#define TCA_CHAIN 11 #define TCA_INGRESS_BLOCK 13 #endif @@ -207,6 +208,10 @@ static void request_from_tcf_id(struct tcf_id *id, uint16_t eth_type, TC_EGRESS_PARENT : ingress_parent; tcmsg->tcm_info = tc_make_handle(id->prio, eth_type); tcmsg->tcm_handle = id->handle; + + if (id->chain) { + nl_msg_put_u32(request, TCA_CHAIN, id->chain); + } } int @@ -286,6 +291,7 @@ tc_add_del_qdisc(int ifindex, bool add, uint32_t block_id, static const struct nl_policy tca_policy[] = { [TCA_KIND] = { .type = NL_A_STRING, .optional = false, }, [TCA_OPTIONS] = { .type = NL_A_NESTED, .optional = false, }, + [TCA_CHAIN] = { .type = NL_A_U32, .optional = true, }, [TCA_STATS] = { .type = NL_A_UNSPEC, .min_len = sizeof(struct tc_stats), .optional = true, }, [TCA_STATS2] = { .type = NL_A_NESTED, .optional = true, }, @@ -1129,12 +1135,13 @@ nl_parse_tcf(const struct tcf_t *tm, struct tc_flower *flower) } static int -nl_parse_act_drop(struct nlattr *options, struct tc_flower *flower) +nl_parse_act_gact(struct nlattr *options, struct tc_flower *flower) { struct nlattr *gact_attrs[ARRAY_SIZE(gact_policy)]; const struct tc_gact *p; struct nlattr *gact_parms; const struct tcf_t *tm; + struct tc_action *action; if (!nl_parse_nested(options, gact_policy, gact_attrs, ARRAY_SIZE(gact_policy))) { @@ -1145,7 +1152,11 @@ nl_parse_act_drop(struct nlattr *options, struct tc_flower *flower) gact_parms = gact_attrs[TCA_GACT_PARMS]; p = nl_attr_get_unspec(gact_parms, sizeof *p); - if (p->action != TC_ACT_SHOT) { + if (TC_ACT_EXT_CMP(p->action, TC_ACT_GOTO_CHAIN)) { + action = &flower->actions[flower->action_count++]; + action->chain = p->action & TC_ACT_EXT_VAL_MASK; + action->type = TC_ACT_GOTO; + } else if (p->action != TC_ACT_SHOT) { VLOG_ERR_RL(&error_rl, "unknown gact action: %d", p->action); return EINVAL; } @@ -1423,7 +1434,7 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower) act_cookie = action_attrs[TCA_ACT_COOKIE]; if (!strcmp(act_kind, "gact")) { - err = nl_parse_act_drop(act_options, flower); + err = nl_parse_act_gact(act_options, flower); } else if (!strcmp(act_kind, "mirred")) { err = nl_parse_act_mirred(act_options, flower); } else if (!strcmp(act_kind, "vlan")) { @@ -1574,6 +1585,10 @@ parse_netlink_to_tc_flower(struct ofpbuf *reply, struct tcf_id *id, return EPROTO; } + if (ta[TCA_CHAIN]) { + id->chain = nl_attr_get_u32(ta[TCA_CHAIN]); + } + kind = nl_attr_get_string(ta[TCA_KIND]); if (strcmp(kind, "flower")) { VLOG_DBG_ONCE("Unsupported filter: %s", kind); @@ -1870,7 +1885,7 @@ nl_msg_put_act_tunnel_key_set(struct ofpbuf *request, bool id_present, } static void -nl_msg_put_act_drop(struct ofpbuf *request) +nl_msg_put_act_gact(struct ofpbuf *request, uint32_t chain) { size_t offset; @@ -1879,6 +1894,10 @@ nl_msg_put_act_drop(struct ofpbuf *request) { struct tc_gact p = { .action = TC_ACT_SHOT }; + if (chain) { + p.action = TC_ACT_GOTO_CHAIN | chain; + } + nl_msg_put_unspec(request, TCA_GACT_PARMS, &p, sizeof p); } nl_msg_end_nested(request, offset); @@ -2224,12 +2243,30 @@ nl_msg_put_flower_acts(struct ofpbuf *request, struct tc_flower *flower) nl_msg_end_nested(request, act_offset); } break; + case TC_ACT_GOTO: { + if (released) { + /* We don't support tunnel release + output + goto + * for now, as next chain by default will try and match + * the tunnel metadata that was released/unset. + * + * This will happen with tunnel + mirror ports. + */ + return -EOPNOTSUPP; + } + + act_offset = nl_msg_start_nested(request, act_index++); + nl_msg_put_act_gact(request, action->chain); + nl_msg_put_act_cookie(request, &flower->act_cookie); + nl_msg_end_nested(request, act_offset); + } + break; } } } - if (!ifindex) { + + if (!flower->action_count) { act_offset = nl_msg_start_nested(request, act_index++); - nl_msg_put_act_drop(request); + nl_msg_put_act_gact(request, 0); nl_msg_put_act_cookie(request, &flower->act_cookie); nl_msg_put_act_flags(request); nl_msg_end_nested(request, act_offset); diff --git a/lib/tc.h b/lib/tc.h index da9a766c6c6f..9154fd889ef9 100644 --- a/lib/tc.h +++ b/lib/tc.h @@ -156,10 +156,13 @@ enum tc_action_type { TC_ACT_MPLS_POP, TC_ACT_MPLS_PUSH, TC_ACT_MPLS_SET, + TC_ACT_GOTO, }; struct tc_action { union { + int chain; + struct { int ifindex_out; bool ingress; @@ -214,6 +217,7 @@ struct tcf_id { enum tc_qdisc_hook hook; uint32_t block_id; int ifindex; + uint32_t chain; uint16_t prio; uint32_t handle; }; @@ -233,6 +237,17 @@ tc_make_tcf_id(int ifindex, uint32_t block_id, uint16_t prio, return id; } +static inline struct tcf_id +tc_make_tcf_id_chain(int ifindex, uint32_t block_id, uint32_t chain, + uint16_t prio, enum tc_qdisc_hook hook) +{ + struct tcf_id id = tc_make_tcf_id(ifindex, block_id, prio, hook); + + id.chain = chain; + + return id; +} + static inline bool is_tcf_id_eq(struct tcf_id *id1, struct tcf_id *id2) { @@ -241,7 +256,8 @@ is_tcf_id_eq(struct tcf_id *id1, struct tcf_id *id2) && id1->handle == id2->handle && id1->hook == id2->hook && id1->block_id == id2->block_id - && id1->ifindex == id2->ifindex; + && id1->ifindex == id2->ifindex + && id1->chain == id2->chain; } struct tc_flower {