Re: [PATCH] mm: Move mm_count into its own cache line

[Mathieu Desnoyers]

On 6/16/23 18:35, John Hubbard wrote:
> On 6/16/23 13:38, Mathieu Desnoyers wrote:
> ...
> > > > This comment is rather odd for a few reasons:
> > > >
> > > > - It requires addition/removal of mm_struct fields to carefully 
> > > > consider
> > > >    field alignment of _other_ fields,
> > > > - It expresses the wish to keep an "optimal" alignment for a specific
> > > >    kernel config.
> > > >
> > > > I suspect that the author of this comment may want to revisit this 
> > > > topic
> > > > and perhaps introduce a split-struct approach for struct rw_semaphore,
> > > > if the need is to place various fields of this structure in different
> > > > cache lines.
>
> Agreed. The whole thing is far too fragile, but when reviewing this I
> wasn't sure what else to suggest. Now looking at it again with your
> alignment suggestion, there is an interesting conflicting set of
> desires:
>
> a) Here: Feng Tang discovered that .count and .owner are best put in
> separate cache lines for the contended case for mmap_lock, and
>
> b) rwsem.h, which specifies precisely the opposite for the uncontended
> case:
>
>   * For an uncontended rwsem, count and owner are the only fields a task
>   * needs to touch when acquiring the rwsem. So they are put next to each
>   * other to increase the chance that they will share the same cacheline.
>
> I suspect that overall, it's "better" to align rw_semaphore's .count and
> .owner field so that the lock is optimized for the contended case,
> because it's reasonable to claim that the benefit of having those two
> fields in the same cacheline for the uncontended case is far less than
> the cost to the contended case, of keeping them close to each other.
>
> However, it's still not unlikely that someone will measure a performance
> regression if such a change is made.
>
> Thoughts?

I suspect that the purpose of b) is only to maximize the functional density
of the data cache: only using a single cache line for the rwsem uncontended
case has the smallest footprint on the data cache.

However, if we look at the rwsem in the context of its use within another
data structure containing it, I think we can do better by analyzing the access
pattern of _other_ fields of that structure.

I have faced a similar situation within liburcu's wfcqueue's API [1], where
it's better for head and tail to sit on different cache lines to eliminate
false-sharing between enqueue and dequeue. I solved this by splitting the
head and tail parameters in the API. So the user can decide to place them
other on the same cache line, or on different cache lines, depending on the
use-case. The user also has the freedom to place both head and tail on the
same cache line as _other_ fields based on usage pattern.

By providing enough flexibility to place the rwsem fields so that the count
is on its own cache-line, and owner is on the same cache line as other fields
touched when the rwsem is held, I suspect we can both improve functional
density _and_ eliminate false-sharing in the contended case.

Thanks,

Mathieu

[1] https://github.com/urcu/userspace-rcu/blob/master/include/urcu/wfcqueue.h#L279


> ...
> > > If the plan is to put mm_count in "its own" cacheline then padding will
> > > be needed?
> >
> > It's taken care of by the anonymous structure trick. Here is an quick 
> > example showing the difference between alignment attribute applied to 
> > an integer type vs to an anonymous structure:
>
> Thanks for explaining very clearly how that works, that's really
> helpful!
> thanks,

--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com