netdev - Re: [ovs-dev] [PATCH net-next v6 05/10] net: openvswitch: optimize flow-mask looking up

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CALDO+Sb2=w_15-R-CtmLHjZ0JdQMzOvhmgnp7TyU-F13w61NAg@mail.gmail.com>
Date:   Mon, 4 Nov 2019 14:20:50 -0800
From:   William Tu <u9012063@...il.com>
To:     Pravin Shelar <pshelar@....org>
Cc:     Tonghao Zhang <xiangxia.m.yue@...il.com>,
        ovs dev <dev@...nvswitch.org>,
        "David S. Miller" <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [ovs-dev] [PATCH net-next v6 05/10] net: openvswitch: optimize
 flow-mask looking up

On Mon, Nov 4, 2019 at 2:10 PM Pravin Shelar <pshelar@....org> wrote:
>
> On Mon, Nov 4, 2019 at 6:00 AM William Tu <u9012063@...il.com> wrote:
> >
> > On Sat, Nov 2, 2019 at 11:50 PM Pravin Shelar <pshelar@....org> wrote:
> > >
> > > On Fri, Nov 1, 2019 at 7:24 AM <xiangxia.m.yue@...il.com> wrote:
> > > >
> > > > From: Tonghao Zhang <xiangxia.m.yue@...il.com>
> > > >
> > > > The full looking up on flow table traverses all mask array.
> > > > If mask-array is too large, the number of invalid flow-mask
> > > > increase, performance will be drop.
> > > >
> > > > One bad case, for example: M means flow-mask is valid and NULL
> > > > of flow-mask means deleted.
> > > >
> > > > +-------------------------------------------+
> > > > | M | NULL | ...                  | NULL | M|
> > > > +-------------------------------------------+
> > > >
> > > > In that case, without this patch, openvswitch will traverses all
> > > > mask array, because there will be one flow-mask in the tail. This
> > > > patch changes the way of flow-mask inserting and deleting, and the
> > > > mask array will be keep as below: there is not a NULL hole. In the
> > > > fast path, we can "break" "for" (not "continue") in flow_lookup
> > > > when we get a NULL flow-mask.
> > > >
> > > >          "break"
> > > >             v
> > > > +-------------------------------------------+
> > > > | M | M |  NULL |...           | NULL | NULL|
> > > > +-------------------------------------------+
> > > >
> > > > This patch don't optimize slow or control path, still using ma->max
> > > > to traverse. Slow path:
> > > > * tbl_mask_array_realloc
> > > > * ovs_flow_tbl_lookup_exact
> > > > * flow_mask_find
> > > >
> > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@...il.com>
> > > > Tested-by: Greg Rose <gvrose8192@...il.com>
> > > > ---
> > > Acked-by: Pravin B Shelar <pshelar@....org>
> >
> > Nack to this patch.
> >
> > It makes the mask cache invalid when moving the flow mask
> > to fill another hole.
> > And the penalty for miss the mask cache is larger than the
> > benefit of this patch (avoiding the NULL flow-mask).
> >
> Hi William,
>
> The cache invalidation would occur in case of changes to flow mask
> list. I think in general datapath processing there should not be too
> many changes to mask list since a mask is typically shared between
> multiple mega flows. Flow-table changes does not always result in mask
> list updates. In last round Tonghao has posted number to to support
> this patch.
> If you are worried about some other use case can you provide
> performance numbers, that way we can take this discussion forward.
>
I see, I don't have any use case to provide.
Thanks,
William