Ryan Roberts <ryan.roberts@xxxxxxx> writes:
> On 21/03/2024 04:39, Huang, Ying wrote:
>> Ryan Roberts <ryan.roberts@xxxxxxx> writes:
>>
>>> Hi Huang, Ying,
>>>
>>>
>>> On 12/03/2024 07:51, Huang, Ying wrote:
>>>> Ryan Roberts <ryan.roberts@xxxxxxx> 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
