RE: [PATCH] ring: add cache guard after ring elements table

[Konstantin Ananyev <konstantin.ananyev@huawei.com>]

> > > > > > > Added cache guard after the table holding the ring elements,
> > to
> > > > avoid
> > > > > > > false sharing conflicts caused by next-line hardware
> > prefetchers
> > > > when
> > > > > > > accessing elements at the end of the ring table.
> > > > > >
> > > > > > I don't see any harm with it, and in theory it might help in
> > some
> > > > > > cases...
> > > > > > Though I wonder how real is that problem?
> > > > > > Did you ever observe such contention to happen?
> > > > >
> > > > > I never observed a problem with this.
> > > > > The risk of contention depends on what is allocated in the memory
> > > > after the ring.
> > > > > Which is application specific.
> > > > >
> > > > > It seems like a purely theoretical issue, but should be fixed
> > anyway,
> > > > to eliminate
> > > > > that risk.
> > > >
> > > > Ok, as I said I see no harm with it.
> > > > Should we document this change somewhere? RN or PG?
> > >
> > > We don't want the release notes overflowing with minor details.
> > > IMO, this change is below the threshold for what people might care
> > about.
> > > People interested in the detailed changes between releases should
> > read the git
> > > log.
> >
> > I still think we do need document somewhere why we doing it.
> > If you think RN or PG is not the right place, let's just put it as a
> > comment for that particular function.
> 
> The function returns the total size of the structure including its elements.
> There's no need to document if the structure includes padding (the trailing cache
> guard) or not.
> The presence of cache guards or other forms of padding is not documented for
> any other "get size" functions, so let's not do it here either.
> 
> Considering how this function is used, I don't see any risk with this patch. So I'd
> rather be silent than add noise.
> If I could see any risk at all, I'd agree it should be mentioned in the RN.
> 
> 
> Maybe I misunderstood your feedback, so here's another angle:
> Do you think we should describe why the cache guard is there?
> Then my response is: The name RTE_CACHE_GUARD_LINES implies that it is a
> cache guard.

Yes, I think we need to describe why we decided that we do need that extra cache-line here.
There was no evidence of any performance hit caused by not having it,
so I think it is worth to put a two line comment to explain why we think
such change would be beneficial.
 
> Digging somewhat more into this... the RTE_CACHE_GUARD_LINES is defined in
> rte_config.h without any description.
> So a description of the concept of a "cache guard" could be added there.
> But there seems to be a tradition for not having any descriptive comments in that
> file. :-(
> 
> There's a good description for the RTE_CACHE_GUARD macro in rte_common.h.
> Maybe we should introduce a similarly well described RTE_CACHE_GUARD_SIZE
> macro in rte_common.h, and use sz += RTE_CACHE_GUARD_SIZE instead of sz +=
> RTE_CACHE_GUARD_LINES * RTE_CACHE_LINE_SIZE.
> 
> >
> > > Also, I don't think it's worth backporting, because I consider it
> > unlikely to have
> > > any real effect.
> > > In the context of backporting, it could be considered a performance
> > > improvement rather than a bug fix.
> >
> > I don't see much point to backport it.
> >
> > >
> > > > Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> > > >
> > > > > >
> > > > > > > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > > > > > > ---
> > > > > > >  lib/ring/rte_ring.c | 3 +++
> > > > > > >  1 file changed, 3 insertions(+)
> > > > > > >
> > > > > > > diff --git a/lib/ring/rte_ring.c b/lib/ring/rte_ring.c
> > > > > > > index f10050a1c4..9ccc62cd42 100644
> > > > > > > --- a/lib/ring/rte_ring.c
> > > > > > > +++ b/lib/ring/rte_ring.c
> > > > > > > @@ -73,8 +73,11 @@ rte_ring_get_memsize_elem(unsigned int
> > esize,
> > > > > > unsigned
> > > > > > > int count)
> > > > > > >  		return -EINVAL;
> > > > > > >  	}
> > > > > > >
> > > > > > > +	static_assert(sizeof(struct rte_ring) ==
> > > > > > > RTE_CACHE_LINE_ROUNDUP(sizeof(struct rte_ring)),
> > > > > > > +			"Size of struct rte_ring not cache line
> > > > aligned");
> > > > > > >  	sz = sizeof(struct rte_ring) + (ssize_t)count * esize;
> > > > > > >  	sz = RTE_ALIGN(sz, RTE_CACHE_LINE_SIZE);
> > > > > > > +	sz += RTE_CACHE_GUARD_LINES * RTE_CACHE_LINE_SIZE;
> > > > > > >  	return sz;
> > > > > > >  }
> > > > > > >
> > > > > > > --
> > > > > > > 2.43.0