From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9EC14C433EF for ; Thu, 4 Nov 2021 18:02:06 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 2D7ED611C0 for ; Thu, 4 Nov 2021 18:02:06 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 2D7ED611C0 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id 9E2506B006C; Thu, 4 Nov 2021 14:02:05 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 992226B0072; Thu, 4 Nov 2021 14:02:05 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 833DE940007; Thu, 4 Nov 2021 14:02:05 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0197.hostedemail.com [216.40.44.197]) by kanga.kvack.org (Postfix) with ESMTP id 72B176B006C for ; Thu, 4 Nov 2021 14:02:05 -0400 (EDT) Received: from smtpin07.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 17D8D7001D for ; Thu, 4 Nov 2021 18:02:05 +0000 (UTC) X-FDA: 78772016610.07.A86A906 Received: from mail-pf1-f174.google.com (mail-pf1-f174.google.com [209.85.210.174]) by imf07.hostedemail.com (Postfix) with ESMTP id 7BDE710004D0 for ; Thu, 4 Nov 2021 18:02:04 +0000 (UTC) Received: by mail-pf1-f174.google.com with SMTP id s5so6622906pfg.2 for ; Thu, 04 Nov 2021 11:02:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=u6YGkFO7tI2Eve3UgZBAq29OlK87/cnZsaZd3QDrJkk=; b=hByAzJSlDCs8xtxF3+0sqOoYXpql74jhRSdwwe+/AnmbL7wza5lry3J/uUUjvWnMR4 MqyOy4GHO0KjWCtDZHgysJ12o/jdd/K0JWBB0GLnjT2KqugCpqngAheK+xrymP6uigmz 0qtmF2HaDqE+6teg747/uD3sIIhqJSgRv9wj3debExaCoGVJAinWvuJBCbb2FIqre6YO yczBoiPWU2X67qoupr2puDIMn/fVu8zUfEvs0odKThJNzU2BFLoGdBfSvPyuglrOEMi+ 4YKYZodkwWwLc0gE/9hZK/IQqJnKBl6nSEiX6n1CVoA7kLBpi9c6hiK+0gu8aAqXNXd8 jn9Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=u6YGkFO7tI2Eve3UgZBAq29OlK87/cnZsaZd3QDrJkk=; b=E7ekUtqbFCW4pLJz66/FVBN8FFUfmXLD2lj/T0bhcw/xeTcTHRZ7oaXc4oxLM9gCef 8Izv+1bx+FLD9fzoaJNFtFoHiEVq5ooI1eJcO2UZPk5fsR72Zx4ldA6Pos1yWC33WLKP k6Q1ypHnZKfF0UYrpK4VTl6P+YNeRpZr3LgiyLl9ksSebvmwn4r/0G/DmD5yluBa0kAP 8q2tYNm29HF+/+TUKubOXiob40VEQDKWlc1F1sbzYRamJUDkfzcOu1K8wnAe/au4Emd5 TwsLKGHxwx9JiJWqsF7imf7XczncYewcJY0DSQAiYHTsyOzJrDMmcdLWKbYHgcpSkUIS RUoQ== X-Gm-Message-State: AOAM5334pbW0xnq6amh2l5c/7gunFz5xTu36BvooaL7/nP0ClGBii+ZN hWPkQ4F3ZcHYW3gX1uhdufMtBivBBNG4OMqSBxPCew== X-Google-Smtp-Source: ABdhPJxW8LYslsDCKLblIKQslkxxhgJgSEgRw8N/MWBoSXVLW9SbhzBAOPOcjeNT4iQu38Q89MAAS7u7rlHuT7BpQtM= X-Received: by 2002:a05:6a00:1c4a:b0:481:ed3:b616 with SMTP id s10-20020a056a001c4a00b004810ed3b616mr30524307pfw.10.1636048922911; Thu, 04 Nov 2021 11:02:02 -0700 (PDT) MIME-Version: 1.0 References: <20211103184708.1778294-1-jimmyshiu@google.com> In-Reply-To: From: Jimmy Shiu Date: Fri, 5 Nov 2021 02:01:51 +0800 Message-ID: Subject: Re: [PATCH] SCHED: attribute page lock and waitqueue functions as sched To: Matthew Wilcox Cc: mingo@redhat.com, joaodias@google.com, wvw@google.com, Minchan Kim , Will McVicker , Peter Zijlstra , Juri Lelli , Vincent Guittot , Dietmar Eggemann , Steven Rostedt , Ben Segall , Mel Gorman , Daniel Bristot de Oliveira , Andrew Morton , David Howells , Vlastimil Babka , William Kucharski , "Kirill A. Shutemov" , Andreas Gruenbacher , linux-kernel@vger.kernel.org, linux-mm@kvack.org Content-Type: multipart/alternative; boundary="000000000000e3596e05cffa5212" X-Rspamd-Server: rspam01 X-Rspamd-Queue-Id: 7BDE710004D0 X-Stat-Signature: f6akxc61c4djyqysne5jp7jnf9nxyikf Authentication-Results: imf07.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=hByAzJSl; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf07.hostedemail.com: domain of jimmyshiu@google.com designates 209.85.210.174 as permitted sender) smtp.mailfrom=jimmyshiu@google.com X-HE-Tag: 1636048924-566955 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: --000000000000e3596e05cffa5212 Content-Type: text/plain; charset="UTF-8" On Thu, Nov 4, 2021 at 2:59 AM Matthew Wilcox wrote: > On Thu, Nov 04, 2021 at 02:47:03AM +0800, Jimmy Shiu wrote: > > Bug: 144961676 > > Bug: 144713689 > > Bug: 172212772 > > A bug number is meaningless without knowing which bug tracker they > refer to. I suggest contemplating the meaning of the 'U' in URL. > > > Signed-off-by: Minchan Kim > > Signed-off-by: Jimmy Shiu > > (cherry picked from commit 1e4de875d9e0cfaccf5131bcc709ae8646cdc168) > > what tree is that commit ID in? I suggest it's meaningless and > should be removed. > > > @@ -687,7 +688,7 @@ static inline void folio_lock(struct folio *folio) > > /* > > * lock_page may only be called if we have the page's inode pinned. > > */ > > -static inline void lock_page(struct page *page) > > +static inline __sched void lock_page(struct page *page) > > Why do you need to tag a static inline function as __sched? This would > be the only place where that is done. > There's no guarantee that any function will be expanded inline. Ex: I checked the System.map, the `folio_wait_bit_common` isn't expanded. > > > +++ b/kernel/sched/wait.c > > @@ -404,7 +404,8 @@ void finish_wait(struct wait_queue_head *wq_head, > struct wait_queue_entry *wq_en > > } > > EXPORT_SYMBOL(finish_wait); > > > > -int autoremove_wake_function(struct wait_queue_entry *wq_entry, > unsigned mode, int sync, void *key) > > +__sched int autoremove_wake_function(struct wait_queue_entry *wq_entry, > unsigned int mode, > > + int sync, void *key) > > { > > This function doesn't sleep. Why does it need to be tagged __sched? > Right, I will remove this. > > > @@ -440,7 +441,7 @@ static inline bool is_kthread_should_stop(void) > > * } smp_mb(); // C > > * remove_wait_queue(&wq_head, &wait); wq_entry->flags |= > WQ_FLAG_WOKEN; > > */ > > -long wait_woken(struct wait_queue_entry *wq_entry, unsigned mode, long > timeout) > > +__sched long wait_woken(struct wait_queue_entry *wq_entry, unsigned int > mode, long timeout) > > This one makes sense, but you've extended the length of the line past 80 > columns, and it seems like it should be a separate patch with its own > justification. > OK, .let me create another patch for this. > > > +__sched int woken_wake_function(struct wait_queue_entry *wq_entry, > unsigned int mode, > > + int sync, void *key) > > This doesn't seem to sleep either? > Right, I will remove this. > > > +++ b/mm/filemap.c > > @@ -1271,7 +1271,7 @@ static inline bool folio_trylock_flag(struct folio > *folio, int bit_nr, > > /* How many times do we accept lock stealing from under a waiter? */ > > int sysctl_page_lock_unfairness = 5; > > > > -static inline int folio_wait_bit_common(struct folio *folio, int bit_nr, > > +static inline __sched int folio_wait_bit_common(struct folio *folio, > int bit_nr, > > int state, enum behavior behavior) > > { > > wait_queue_head_t *q = folio_waitqueue(folio); > > @@ -1411,13 +1411,13 @@ static inline int folio_wait_bit_common(struct > folio *folio, int bit_nr, > > return wait->flags & WQ_FLAG_WOKEN ? 0 : -EINTR; > > } > > > > -void folio_wait_bit(struct folio *folio, int bit_nr) > > +__sched void folio_wait_bit(struct folio *folio, int bit_nr) > > { > > folio_wait_bit_common(folio, bit_nr, TASK_UNINTERRUPTIBLE, SHARED); > > } > > EXPORT_SYMBOL(folio_wait_bit); > > > > -int folio_wait_bit_killable(struct folio *folio, int bit_nr) > > +__sched int folio_wait_bit_killable(struct folio *folio, int bit_nr) > > { > > return folio_wait_bit_common(folio, bit_nr, TASK_KILLABLE, SHARED); > > } > > @@ -1628,21 +1628,21 @@ EXPORT_SYMBOL_GPL(page_endio); > > * __folio_lock - Get a lock on the folio, assuming we need to sleep to > get it. > > * @folio: The folio to lock > > */ > > -void __folio_lock(struct folio *folio) > > +__sched void __folio_lock(struct folio *folio) > > { > > folio_wait_bit_common(folio, PG_locked, TASK_UNINTERRUPTIBLE, > > EXCLUSIVE); > > } > > EXPORT_SYMBOL(__folio_lock); > > > > -int __folio_lock_killable(struct folio *folio) > > +__sched int __folio_lock_killable(struct folio *folio) > > { > > return folio_wait_bit_common(folio, PG_locked, TASK_KILLABLE, > > EXCLUSIVE); > > } > > EXPORT_SYMBOL_GPL(__folio_lock_killable); > > > > -static int __folio_lock_async(struct folio *folio, struct > wait_page_queue *wait) > > +static __sched int __folio_lock_async(struct folio *folio, struct > wait_page_queue *wait) > > { > > struct wait_queue_head *q = folio_waitqueue(folio); > > int ret = 0; > > @@ -1679,7 +1679,7 @@ static int __folio_lock_async(struct folio *folio, > struct wait_page_queue *wait) > > * If neither ALLOW_RETRY nor KILLABLE are set, will always return true > > * with the folio locked and the mmap_lock unperturbed. > > */ > > -bool __folio_lock_or_retry(struct folio *folio, struct mm_struct *mm, > > +__sched bool __folio_lock_or_retry(struct folio *folio, struct > mm_struct *mm, > > unsigned int flags) > > { > > if (fault_flag_allow_retry_first(flags)) { > > -- > > 2.34.0.rc0.344.g81b53c2807-goog > > > > > --000000000000e3596e05cffa5212 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Thu, Nov 4, 2021 at 2:59 AM Matthe= w Wilcox <willy@infradead.org= > wrote:
On T= hu, Nov 04, 2021 at 02:47:03AM +0800, Jimmy Shiu wrote:
> Bug: 144961676
> Bug: 144713689
> Bug: 172212772

A bug number is meaningless without knowing which bug tracker they
refer to.=C2=A0 I suggest contemplating the meaning of the 'U' in U= RL.

> Signed-off-by: Minchan Kim <minchan@google.com>
> Signed-off-by: Jimmy Shiu <jimmyshiu@google.com>
> (cherry picked from commit 1e4de875d9e0cfaccf5131bcc709ae8646cdc168)
what tree is that commit ID in?=C2=A0 I suggest it's meaningless and should be removed.

> @@ -687,7 +688,7 @@ static inline void folio_lock(struct folio *folio)=
>=C2=A0 /*
>=C2=A0 =C2=A0* lock_page may only be called if we have the page's i= node pinned.
>=C2=A0 =C2=A0*/
> -static inline void lock_page(struct page *page)
> +static inline __sched void lock_page(struct page *page)

Why do you need to tag a static inline function as __sched?=C2=A0 This woul= d
be the only place where that is done.
There's no guarant= ee that any function will be expanded inline.
Ex: I checked the System.map, the `folio_wait_bit_common` isn&#= 39;t expanded.=C2=A0

> +++ b/kernel/sched/wait.c
> @@ -404,7 +404,8 @@ void finish_wait(struct wait_queue_head *wq_head, = struct wait_queue_entry *wq_en
>=C2=A0 }
>=C2=A0 EXPORT_SYMBOL(finish_wait);
>=C2=A0
> -int autoremove_wake_function(struct wait_queue_entry *wq_entry, unsig= ned mode, int sync, void *key)
> +__sched int autoremove_wake_function(struct wait_queue_entry *wq_entr= y, unsigned int mode,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 int sync, void *key)
>=C2=A0 {

This function doesn't sleep.=C2=A0 Why does it need to be tagged __sche= d?
Right, I will remove this.=C2=A0

> @@ -440,7 +441,7 @@ static inline bool is_kthread_should_stop(void) >=C2=A0 =C2=A0* }=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0smp_mb(); // C
>=C2=A0 =C2=A0* remove_wait_queue(&wq_head, &wait);=C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0wq_entry->flags |=3D WQ_FLAG_W= OKEN;
>=C2=A0 =C2=A0*/
> -long wait_woken(struct wait_queue_entry *wq_entry, unsigned mode, lon= g timeout)
> +__sched long wait_woken(struct wait_queue_entry *wq_entry, unsigned i= nt mode, long timeout)

This one makes sense, but you've extended the length of the line past 8= 0
columns, and it seems like it should be a separate patch with its own
justification.
OK, .let me create another patch for this.=C2=A0

> +__sched int woken_wake_function(struct wait_queue_entry *wq_entry, un= signed int mode,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0int sync, void *key)

This doesn't seem to sleep either?
Right, I will re= move this.=C2=A0=C2=A0

> +++ b/mm/filemap.c
> @@ -1271,7 +1271,7 @@ static inline bool folio_trylock_flag(struct fol= io *folio, int bit_nr,
>=C2=A0 /* How many times do we accept lock stealing from under a waiter= ? */
>=C2=A0 int sysctl_page_lock_unfairness =3D 5;
>=C2=A0
> -static inline int folio_wait_bit_common(struct folio *folio, int bit_= nr,
> +static inline __sched int folio_wait_bit_common(struct folio *folio, = int bit_nr,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0int state, enum = behavior behavior)
>=C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0wait_queue_head_t *q =3D folio_waitqueue(fol= io);
> @@ -1411,13 +1411,13 @@ static inline int folio_wait_bit_common(struct= folio *folio, int bit_nr,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0return wait->flags & WQ_FLAG_WOKEN ? = 0 : -EINTR;
>=C2=A0 }
>=C2=A0
> -void folio_wait_bit(struct folio *folio, int bit_nr)
> +__sched void folio_wait_bit(struct folio *folio, int bit_nr)
>=C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0folio_wait_bit_common(folio, bit_nr, TASK_UN= INTERRUPTIBLE, SHARED);
>=C2=A0 }
>=C2=A0 EXPORT_SYMBOL(folio_wait_bit);
>=C2=A0
> -int folio_wait_bit_killable(struct folio *folio, int bit_nr)
> +__sched int folio_wait_bit_killable(struct folio *folio, int bit_nr)<= br> >=C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0return folio_wait_bit_common(folio, bit_nr, = TASK_KILLABLE, SHARED);
>=C2=A0 }
> @@ -1628,21 +1628,21 @@ EXPORT_SYMBOL_GPL(page_endio);
>=C2=A0 =C2=A0* __folio_lock - Get a lock on the folio, assuming we need= to sleep to get it.
>=C2=A0 =C2=A0* @folio: The folio to lock
>=C2=A0 =C2=A0*/
> -void __folio_lock(struct folio *folio)
> +__sched void __folio_lock(struct folio *folio)
>=C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0folio_wait_bit_common(folio, PG_locked, TASK= _UNINTERRUPTIBLE,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0EXCLUSIVE);
>=C2=A0 }
>=C2=A0 EXPORT_SYMBOL(__folio_lock);
>=C2=A0
> -int __folio_lock_killable(struct folio *folio)
> +__sched int __folio_lock_killable(struct folio *folio)
>=C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0return folio_wait_bit_common(folio, PG_locke= d, TASK_KILLABLE,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0EXCLUS= IVE);
>=C2=A0 }
>=C2=A0 EXPORT_SYMBOL_GPL(__folio_lock_killable);
>=C2=A0
> -static int __folio_lock_async(struct folio *folio, struct wait_page_q= ueue *wait)
> +static __sched int __folio_lock_async(struct folio *folio, struct wai= t_page_queue *wait)
>=C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0struct wait_queue_head *q =3D folio_waitqueu= e(folio);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0int ret =3D 0;
> @@ -1679,7 +1679,7 @@ static int __folio_lock_async(struct folio *foli= o, struct wait_page_queue *wait)
>=C2=A0 =C2=A0* If neither ALLOW_RETRY nor KILLABLE are set, will always= return true
>=C2=A0 =C2=A0* with the folio locked and the mmap_lock unperturbed.
>=C2=A0 =C2=A0*/
> -bool __folio_lock_or_retry(struct folio *folio, struct mm_struct *mm,=
> +__sched bool __folio_lock_or_retry(struct folio *folio, struct mm_str= uct *mm,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 unsigned int flags)
>=C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (fault_flag_allow_retry_first(flags)) { > --
> 2.34.0.rc0.344.g81b53c2807-goog
>
>
--000000000000e3596e05cffa5212--