Hi sorry for late reply, didn't get this email.
On 03.12.2019 16:16, Ilya Maximets wrote:
> From: Ilya Maximets <i.maxim...@samsung.com>
> On 03.12.2019 14:45, Roi Dayan wrote:
> > From: Paul Blakey <pa...@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.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
