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

[Morten Brørup]

> > > > > > 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.

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 at huawei.com>
> > >
> > > > >
> > > > > > Signed-off-by: Morten Brørup <mb at 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