Re: [PATCH] Revert "mm/vunmap: add cond_resched() in vunmap_pmd_range"

From: Andrew Morton <akpm@linux-foundation.org>
To: Minchan Kim <minchan@kernel.org>
Cc: linux-kernel@vger.kernel.org, linux-mm <linux-mm@kvack.org>,
	"Aneesh Kumar K . V" <aneesh.kumar@linux.ibm.com>,
	Harish Sriram <harish@linux.ibm.com>,
	stable@vger.kernel.org
Subject: Re: [PATCH] Revert "mm/vunmap: add cond_resched() in vunmap_pmd_range"
Date: Thu, 12 Nov 2020 14:49:19 -0800	[thread overview]
Message-ID: <20201112144919.5f6b36876f4e59ebb4a99d6d@linux-foundation.org> (raw)
In-Reply-To: <20201112200101.GC123036@google.com>

On Thu, 12 Nov 2020 12:01:01 -0800 Minchan Kim <minchan@kernel.org> wrote:

> 
> On Sat, Nov 07, 2020 at 12:39:39AM -0800, Minchan Kim wrote:
> > Hi Andrew,
> > 
> > On Fri, Nov 06, 2020 at 05:59:33PM -0800, Andrew Morton wrote:
> > > On Thu,  5 Nov 2020 09:02:49 -0800 Minchan Kim <minchan@kernel.org> wrote:
> > > 
> > > > This reverts commit e47110e90584a22e9980510b00d0dfad3a83354e.
> > > > 
> > > > While I was doing zram testing, I found sometimes decompression failed
> > > > since the compression buffer was corrupted. With investigation,
> > > > I found below commit calls cond_resched unconditionally so it could
> > > > make a problem in atomic context if the task is reschedule.
> > > > 
> > > > Revert the original commit for now.
>
> How should we proceed this problem?
>

(top-posting repaired - please don't).

Well, we don't want to reintroduce the softlockup reports which
e47110e90584a2 fixed, and we certainly don't want to do that on behalf
of code which is using the unmap_kernel_range() interface incorrectly.

So I suggest either

a) make zs_unmap_object() stop doing the unmapping from atomic context or

b) figure out whether the vmalloc unmap code is *truly* unsafe from
   atomic context - perhaps it is only unsafe from interrupt context,
   in which case we can rework the vmalloc.c checks to reflect this, or

c) make the vfree code callable from all contexts.  Which is by far
   the preferred solution, but may be tough.

Or maybe not so tough - if the only problem in the vmalloc code is the
use of mutex_trylock() from irqs then it may be as simple as switching
to old-style semaphores and using down_trylock(), which I think is
irq-safe.

However old-style semaphores are deprecated.  A hackyish approach might
be to use an rwsem always in down_write mode and use
down_write_trylock(), which I think is also callable from interrrupt
context.

But I have a feeling that there are other reasons why vfree shouldn't
be called from atomic context, apart from the mutex_trylock-in-irq
issue.