[ovs-dev] [PATCH] flow: Read recirculation depth once for whole batch in miniflow_extract

[Gaëtan Rivet]

On Tue, Apr 13, 2021, at 13:19, Balazs Nemeth wrote:
> On Sun, 2021-04-11 at 22:27 +0200, Gaëtan Rivet wrote:
> > On Fri, Apr 9, 2021, at 20:27, Gregory Rose wrote:
> > > 
> > > 
> > > On 4/9/2021 3:09 AM, Balazs Nemeth wrote:
> > > > The call to recirc_depth_get involves accessing a TLS value which
> > > > is
> > > > slower than accessing the stack.
> > > > 
> > > > Signed-off-by: Balazs Nemeth <bnemeth at redhat.com>
> > > > ---
> > > >   lib/dpif-netdev.c | 3 ++-
> > > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > > > index 251788b04..41ac500a1 100644
> > > > --- a/lib/dpif-netdev.c
> > > > +++ b/lib/dpif-netdev.c
> > > > @@ -7095,6 +7095,7 @@ dfc_processing(struct dp_netdev_pmd_thread
> > > > *pmd,
> > > >       struct dp_packet *packet;
> > > >       const size_t cnt = dp_packet_batch_size(packets_);
> > > >       uint32_t cur_min = pmd->ctx.emc_insert_min;
> > > > +    const uint32_t depth = *recirc_depth_get();
> > > >       int i;
> > > >       uint16_t tcp_flags;
> > > >       bool smc_enable_db;
> > > > @@ -7127,7 +7128,7 @@ dfc_processing(struct dp_netdev_pmd_thread
> > > > *pmd,
> > > >               pkt_metadata_init(&packet->md, port_no);
> > > >           }
> > > >   
> > > > -        if ((*recirc_depth_get() == 0) &&
> > > > +        if (depth == 0 &&
> > > >               dp_packet_has_flow_mark(packet, &mark)) {
> > > >               flow = mark_to_flow_find(pmd, mark);
> > > >               if (OVS_LIKELY(flow)) {
> > > > 
> > > 
> > > Makes sense.
> > > 
> > > Acked-by: Greg Rose <gvrose8192 at gmail.com>
> > 
> > Hi,
> > 
> > How much of a difference does it make? Is it measurable?
> 
> 
> Hi,
> 
> It's definitely measurable. On my setup (similar to pvp with one vm
> running dpdk and 2 flows in ovs to forward packets, all running on an
> arm server), this patch shows an improvement of ~1.6% in throughput.
> 
> 
> > 
> > Otherwise, I know 'depth' has been used in other places for a pointer
> > to this TLS value,
> > but 'recirc_depth' might be clearer to a reader?
> > 
> 
> 
> By reading the TLS variable and storing it in a const variable on the
> stack explicitly communicates to the reader that the value doesn't
> change while processing the current batch of packets. I feel that, in
> this case, it actually makes the code a bit easier to understand.
> 
> Let me know what you think,
> Balazs
> 

No I meant the name of the const variable holding the value.

Instead of:
    const uint32_t depth = *recirc_depth_get();
Using
    const uint32_t recirc_depth = *recirc_depth_get();

It's only a matter of naming, but I think using 'recirc_depth' will be easier to read in the batch loop.
Otherwise indeed the risk was that somewhere during the processing the value was being changed,
AFAIK it doesn't happen and your change is correct.

-- 
Gaetan Rivet