[ovs-dev] [PATCH v3 07/10] netdev-offload-tc: Add recirculation, support via tc chains

Ilya Maximets i.maximets at ovn.org
Fri Dec 13 12:05:12 UTC 2019


> 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.


More information about the dev mailing list