[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2093077.OBFZWjSADL@suse>
Date: Tue, 06 Dec 2022 20:12:13 +0100
From: "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc: Jonathan Corbet <corbet@....net>,
Andrew Morton <akpm@...ux-foundation.org>,
Ira Weiny <ira.weiny@...el.com>,
Mike Rapoport <rppt@...nel.org>, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-mm@...ck.org,
Mike Rapoport <rppt@...ux.ibm.com>,
Peter Zijlstra <a.p.zijlstra@...llo.nl>,
Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH] mm/highmem: Add notes about conversions from kmap{,_atomic}()
On martedì 6 dicembre 2022 09:00:10 CET Sebastian Andrzej Siewior wrote:
> On 2022-12-06 08:00:29 [+0100], Fabio M. De Francesco wrote:
> > diff --git a/Documentation/mm/highmem.rst b/Documentation/mm/highmem.rst
> > index 0f731d9196b0..9523e92299f6 100644
> > --- a/Documentation/mm/highmem.rst
> > +++ b/Documentation/mm/highmem.rst
> > @@ -100,10 +101,21 @@ list shows them in order of preference of use.
> >
> > (included in the "Functions" section) for details on how to manage
nested
> > mappings.
> >
> > -* kmap_atomic(). This permits a very short duration mapping of a single
> > - page. Since the mapping is restricted to the CPU that issued it, it
> > - performs well, but the issuing task is therefore required to stay on
that
> > - CPU until it has finished, lest some other task displace its mappings.
> > +* kmap_atomic(). This function has been deprecated; use
kmap_local_page().
> > +
> > + NOTE: Conversions to kmap_local_page() must take care to follow the
> > mapping + restrictions imposed on kmap_local_page(). Furthermore, code
> > between the + map/unmap operations may implicitly depended on the side
> > effects of + kmap_atomic(), such as disabling pagefaults, migration,
> > and/or preemption. + Such conversions should be changed to make explicit
> > calls for those + requirements.
Sebastian, thanks for taking a look at my patch and replying.
> Furthermore, code between the kmap_atomic() and kunmap_atomic()
> functions may implicitly depended
I suppose it should be "depend"? Shouldn't it?
> on the side effects of kmap_atomic()
> namely disabling pagefaults or preemption or both.
I agree with you for rephrasing, mainly because it is
written in poor English.
However, I still have doubts about why you deleted "migration".
AFAIK, __kmap_local_pfn_prot() always takes care of disabling migration for
HIGHMEM enabled kernels.
How about !HIGHMEM, where kmap_local_page() is an indirect call to
page_address()? Did you mean that, if the code between kmap_atomic() and
kunmap_atomic() depended on migrate_disable() (in PREEMPT_RT) we should always
just stay safe and call preempt_disable() together with conversion to
kmap_local_page()?
If so, I understand and I again agree with you. If not, I'm missing something;
so please let me understand properly.
Aside from the above, I'm not sure whether you deleted the last phrase before
your suggestion. What about making it to become "For the above-mentioned
cases, conversions should also explicitly disable page-faults and/or
preemption"?
Thanks again for noticing my mistakes.
Fabio
>
> > + [Legacy documentation]
> > +
> > + This permits a very short duration mapping of a single page. Since the
> > + mapping is restricted to the CPU that issued it, it performs well, but
> > + the issuing task is therefore required to stay on that CPU until it has
> > + finished, lest some other task displace its mappings.
> >
> > kmap_atomic() may also be used by interrupt contexts, since it does not
> > sleep and the callers too may not sleep until after kunmap_atomic() is
>
> Sebastian
Powered by blists - more mailing lists