> > > > > > > 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 <[email protected]> > > > > > > > > > > > > > > > > > Signed-off-by: Morten Brørup <[email protected]> > > > > > > > --- > > > > > > > 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
