lkml.org 
[lkml]   [2024]   [Mar]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v4 4/6] mm: swap: Allow storage of all mTHP orders
    Date
    Ryan Roberts <ryan.roberts@arm.com> writes:

    > On 21/03/2024 04:39, Huang, Ying wrote:
    >> Ryan Roberts <ryan.roberts@arm.com> writes:
    >>
    >>> Hi Huang, Ying,
    >>>
    >>>
    >>> On 12/03/2024 07:51, Huang, Ying wrote:
    >>>> Ryan Roberts <ryan.roberts@arm.com> writes:
    >>> [...]
    >>>
    >>>>> @@ -905,17 +961,18 @@ static int scan_swap_map_slots(struct swap_info_struct *si,
    >>>>> }
    >>>>>
    >>>>> if (si->swap_map[offset]) {
    >>>>> + VM_WARN_ON(order > 0);
    >>>>> unlock_cluster(ci);
    >>>>> if (!n_ret)
    >>>>> goto scan;
    >>>>> else
    >>>>> goto done;
    >>>>> }
    >>>>> - WRITE_ONCE(si->swap_map[offset], usage);
    >>>>> - inc_cluster_info_page(si, si->cluster_info, offset);
    >>>>> + memset(si->swap_map + offset, usage, nr_pages);
    >>>>
    >>>> Add barrier() here corresponds to original WRITE_ONCE()?
    >>>> unlock_cluster(ci) may be NOP for some swap devices.
    >>>
    >>> Looking at this a bit more closely, I'm not sure this is needed. Even if there
    >>> is no cluster, the swap_info is still locked, so unlocking that will act as a
    >>> barrier. There are a number of other callsites that memset(si->swap_map) without
    >>> an explicit barrier and with the swap_info locked.
    >>>
    >>> Looking at the original commit that added the WRITE_ONCE() it was worried about
    >>> a race with reading swap_map in _swap_info_get(). But that site is now annotated
    >>> with a data_race(), which will suppress the warning. And I don't believe there
    >>> are any places that read swap_map locklessly and depend upon observing ordering
    >>> between it and other state? So I think the si unlock is sufficient?
    >>>
    >>> I'm not planning to add barrier() here. Let me know if you disagree.
    >>
    >> swap_map[] may be read locklessly in swap_offset_available_and_locked()
    >> in parallel. IIUC, WRITE_ONCE() here is to make the writing take effect
    >> as early as possible there.
    >
    > Afraid I'm not convinced by that argument; if it's racing, it's racing - the

    It's not a race.

    > lockless side needs to be robust (it is). Adding the compiler barrier limits the
    > compiler's options which could lead to slower code in this path. If your
    > argument is that you want to reduce the window where
    > swap_offset_available_and_locked() could observe a free swap slot but then see
    > that its taken after it gets the si lock, that seems like a micro-optimization
    > to me, which we should avoid if we can.

    Yes. I think that it is a micro-optimization too. I had thought that
    it is a common practice to use WRITE_ONCE()/READ_ONCE() or barrier() in
    intentional racy data accessing to make the change available as soon as
    possible. But I may be wrong here.

    > By remnoving the WRITE_ONCE() and using memset, the lockless reader could
    > observe tearing though. I don't think that should cause a problem (because
    > everything is rechecked with under the lock), but if we want to avoid it, then
    > perhaps we just need to loop over WRITE_ONCE() here instead of using memset?

    IIUC, in practice that isn't necessary, because type of si->swap_map[]
    is "unsigned char". It isn't possible to tear "unsigned char". In
    theory, it may be better to use WRITE_ONCE() because we may change the
    type of si->swap_map[] at some time (who knows). I don't have a strong
    opinion here.

    >>>>> + add_cluster_info_page(si, si->cluster_info, offset, nr_pages);
    >>>>> unlock_cluster(ci);

    --
    Best Regards,
    Huang, Ying

    \
     
     \ /
      Last update: 2024-05-27 15:58    [W:3.755 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site