Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 24 Jul 2020 17:24:25 +0200
From:   Oleg Nesterov <oleg@redhat.com>
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Hugh Dickins <hughd@google.com>, Michal Hocko <mhocko@kernel.org>,
        Linux-MM <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Tim Chen <tim.c.chen@linux.intel.com>,
        Michal Hocko <mhocko@suse.com>
Subject: Re: [RFC PATCH] mm: silence soft lockups from unlock_page
Message-ID: <20200724152424.GC17209@redhat.com>
References: <CAHk-=whewL14RgwLZTXcNAnrDPt0H+sRJS6iDq0oGb6zwaBMxg@mail.gmail.com>
 <CAHk-=whb0=rjc1WR+F_r_syw5Ld4=ebuNJmmpaPEzfjZRD5Y-w@mail.gmail.com>
 <alpine.LSU.2.11.2007221359450.1017@eggly.anvils>
 <CAHk-=wi=vuc6sdu0m9nYd3gb8x5Xgnc6=TH=DTOy7qU96rZ9nw@mail.gmail.com>
 <CAHk-=whEjnsANEhTA3aqpNLZ3vv7huP7QAmcAEd-GUxm2YMo-Q@mail.gmail.com>
 <20200723124749.GA7428@redhat.com>
 <CAHk-=wgyc7en4=HddEYiz_RKJXfqe1JYv3BzHc=+_wYq9ti+LQ@mail.gmail.com>
 <CAHk-=whQK3OGwExTzCrwwvuuVaQAgs8KsR-Yv8m1BmXoNZZ=jQ@mail.gmail.com>
 <alpine.LSU.2.11.2007231549540.1016@eggly.anvils>
 <CAHk-=wgvGOnMF0ePU4xS236bOsP8jouj3rps+ysCaGXvCjh2Dg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHk-=wgvGOnMF0ePU4xS236bOsP8jouj3rps+ysCaGXvCjh2Dg@mail.gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 07/23, Linus Torvalds wrote:
>
> But I'll walk over my patch mentally one more time. Here's the current
> version, anyway.

Both patches look correct to me, feel free to add

Reviewed-by: Oleg Nesterov <oleg@redhat.com>

> @@ -1013,18 +1014,40 @@ static int wake_page_function(wait_queue_entry_t *wait, unsigned mode, int sync,
>  	if (wait_page->bit_nr != key->bit_nr)
>  		return 0;
>
> +	/* Stop walking if it's locked */
> +	if (wait->flags & WQ_FLAG_EXCLUSIVE) {
> +		if (test_and_set_bit(key->bit_nr, &key->page->flags))
> +			return -1;
> +	} else {
> +		if (test_bit(key->bit_nr, &key->page->flags))
> +			return -1;
> +	}

not sure this makes any sense, but this looks like another user of
trylock_page_bit_common(), see the patch below on top of 1/2.

Oleg.

--- mm/filemap.c~	2020-07-24 17:09:34.728133846 +0200
+++ mm/filemap.c	2020-07-24 17:23:52.279185374 +0200
@@ -1000,6 +1000,14 @@
 	wait_queue_entry_t wait;
 };
 
+static int trylock_page_bit_common(struct page *page, int bit_nr,
+					struct wait_queue_entry *wait)
+{
+	return wait->flags & WQ_FLAG_EXCLUSIVE ?
+		!test_and_set_bit(bit_nr, &page->flags) :
+		!test_bit(bit_nr, &page->flags);
+}
+
 static int wake_page_function(wait_queue_entry_t *wait, unsigned mode, int sync, void *arg)
 {
 	int ret;
@@ -1015,13 +1023,8 @@
 		return 0;
 
 	/* Stop walking if it's locked */
-	if (wait->flags & WQ_FLAG_EXCLUSIVE) {
-		if (test_and_set_bit(key->bit_nr, &key->page->flags))
-			return -1;
-	} else {
-		if (test_bit(key->bit_nr, &key->page->flags))
-			return -1;
-	}
+	if (!trylock_page_bit_common(key->page, key->bit_nr, wait))
+		return -1;
 
 	/*
 	 * Let the waiter know we have done the page flag
@@ -1126,14 +1129,6 @@
 			 */
 };
 
-static inline int trylock_page_bit_common(struct page *page, int bit_nr,
-	enum behavior behavior)
-{
-	return behavior == EXCLUSIVE ?
-		!test_and_set_bit(bit_nr, &page->flags) :
-		!test_bit(bit_nr, &page->flags);
-}
-
 static inline int wait_on_page_bit_common(wait_queue_head_t *q,
 	struct page *page, int bit_nr, int state, enum behavior behavior)
 {
@@ -1170,7 +1165,7 @@
 	 */
 	spin_lock_irq(&q->lock);
 	SetPageWaiters(page);
-	if (!trylock_page_bit_common(page, bit_nr, behavior))
+	if (!trylock_page_bit_common(page, bit_nr, wait))
 		__add_wait_queue_entry_tail(q, wait);
 	spin_unlock_irq(&q->lock);