linux-kernel - Re: [PATCH v4 4/6] mm: swap: Allow storage of all mTHP orders

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87plvnq9ap.fsf@yhuang6-desk2.ccr.corp.intel.com>
Date: Fri, 22 Mar 2024 10:39:42 +0800
From: "Huang, Ying" <ying.huang@...el.com>
To: Ryan Roberts <ryan.roberts@....com>, "Paul E. McKenney"
 <paulmck@...ux.vnet.ibm.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,  David Hildenbrand
 <david@...hat.com>,  Matthew Wilcox <willy@...radead.org>,  Gao Xiang
 <xiang@...nel.org>,  Yu Zhao <yuzhao@...gle.com>,  Yang Shi
 <shy828301@...il.com>,  Michal Hocko <mhocko@...e.com>,  Kefeng Wang
 <wangkefeng.wang@...wei.com>,  Barry Song <21cnbao@...il.com>,  Chris Li
 <chrisl@...nel.org>,  <linux-mm@...ck.org>,
  <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 4/6] mm: swap: Allow storage of all mTHP orders

Ryan Roberts <ryan.roberts@....com> writes:

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