[ovs-dev] [PATCH] lib/netdev-tc-offloads: Fix frag first/later translation

[Simon Horman]

On Thu, Mar 29, 2018 at 03:46:00PM +0300, Roi Dayan wrote:
> 
> 
> On 28/03/2018 15:54, Simon Horman wrote:
> > On Sun, Mar 25, 2018 at 12:53:25PM +0300, Roi Dayan wrote:
> > > 
> > > 
> > > On 25/03/2018 12:11, Roi Dayan wrote:
> > > > Fragment mask (any and later) always exists so we need to test
> > > > for FLOW_NW_FRAG_LATER only if the state is FLOW_NW_FRAG_ANY.
> > > > Before this fix we could pass frag no and first at the same time to TC
> > > > which is also not tested there for bad frag state.
> > > > This fix make sure we only pass frag first/later if is frag.
> > > > 
> > > > Fixes: 83e866067ea6 ("netdev-tc-offloads: Add support for IP fragmentation")
> > > > Signed-off-by: Roi Dayan <roid at mellanox.com>
> > > > Reviewed-by: Paul Blakey <paulb at mellanox.com>
> > > > ---
> > > >    lib/netdev-tc-offloads.c | 19 +++++++++++++------
> > > >    1 file changed, 13 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
> > > > index f22415ee11bd..6db76801fd0e 100644
> > > > --- a/lib/netdev-tc-offloads.c
> > > > +++ b/lib/netdev-tc-offloads.c
> > > > @@ -948,14 +948,21 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
> > > >            flower.key.ip_ttl = key->nw_ttl;
> > > >            flower.mask.ip_ttl = mask->nw_ttl;
> > > > -        if (mask->nw_frag) {
> > > > -            if (key->nw_frag & FLOW_NW_FRAG_ANY)
> > > > +        if (mask->nw_frag & FLOW_NW_FRAG_ANY) {
> > > > +            flower.mask.flags |= TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT;
> > > > +
> > > > +            if (key->nw_frag & FLOW_NW_FRAG_ANY) {
> > > >                    flower.key.flags |= TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT;
> > > > -            if (!(key->nw_frag & FLOW_NW_FRAG_LATER))
> > > > -                flower.key.flags |= TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST;
> > > > -            flower.mask.flags |= TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT;
> > > > -            flower.mask.flags |= TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST;
> > > > +                if (mask->nw_frag & FLOW_NW_FRAG_LATER) {
> > > > +                    flower.mask.flags |= TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST;
> > > > +
> > > > +                    if (!(key->nw_frag & FLOW_NW_FRAG_LATER)) {
> > > > +                        flower.key.flags |= TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST;
> > > > +                    }
> > > > +                }
> > > > +            }
> > > > +
> > > 
> > > Hi Simon,
> > > 
> > > This fix OVS to send TC nofrag without frag first/later.
> > > Though for frag it will set TC frag+fragfirst flags.
> > > This is working fine though from your patch to iproute user-space
> > > you showed an example of using only firstfrag flag without the need
> > > for frag flag.
> > > I'm not sure if we need to minimize it from OVS as well or just let
> > > this be frag+firstfrag.
> > > let me know what you think.
> > 
> > Hi Roi,
> > 
> > thanks for reporting this. I'd like a little more time to investigate this
> > and follow-up properly.
> > 
> 
> ok thanks. let me know if you have some questions or need some output
> examples.

Thanks for the patch and sorry once again for the delay.

I wanted to spend some time testing this patch and analysing its
correctness. I've now satisfied myself on both counts and have committed
the patch to the master branch.