Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To:     Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc:     Ira Weiny <ira.weiny@intel.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        "Matthew Wilcox (Oracle)" <willy@infradead.org>,
        Will Deacon <will@kernel.org>,
        Peter Collingbourne <pcc@google.com>,
        Vlastimil Babka <vbabka@suse.cz>, linux-kernel@vger.kernel.org,
        linux-doc@vger.kernel.org, outreachy@lists.linux.dev,
        Mike Rapoport <rppt@linux.ibm.com>
Subject: Re: [PATCH v3 1/4] mm/highmem: Fix kernel-doc warnings in highmem*.h
Date:   Thu, 28 Apr 2022 12:54:14 +0200
Message-ID: <2598013.X9hSmTKtgW@leap>
In-Reply-To: <YmpaUgxIk0rrwt2G@linutronix.de>
References: <20220427183821.1979-1-fmdefrancesco@gmail.com> <20220427183821.1979-2-fmdefrancesco@gmail.com> <YmpaUgxIk0rrwt2G@linutronix.de>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="utf-8"
Precedence: bulk

On gioved=C3=AC 28 aprile 2022 11:11:46 CEST Sebastian Andrzej Siewior wrot=
e:
> On 2022-04-27 20:38:18 [+0200], Fabio M. De Francesco wrote:
> > --- a/include/linux/highmem-internal.h
> > +++ b/include/linux/highmem-internal.h
> > @@ -236,9 +236,18 @@ static inline unsigned long totalhigh_pages(void)=
=20
{ return 0UL; }
> > =20
> >  #endif /* CONFIG_HIGHMEM */
> > =20
> > -/*
> > - * Prevent people trying to call kunmap_atomic() as if it were=20
kunmap()
> > - * kunmap_atomic() should get the return value of kmap_atomic, not the=
=20
page.
> > +/**
> > + * kunmap_atomic - Unmap the virtual address mapped by kmap_atomic() -=
=20
deprecated!
> > + * @__addr:       Virtual address to be unmapped
> > + *
> > + * Unmaps an address previously mapped by kmap_atomic() and re-enables
> > + * pagefaults, migration, preemption (the latter was disabled only for
> > + * !PREEMP_RT configurations). Mappings should be unmapped in the=20
reverse
>=20
> Not sure how detailed you want to put it here as "reverses kmap_atomic()
> doing." might be okay ;)

No, it's not sufficient because Matthew Wilcox said that something like "It=
=20
is the counterpart of kmap_atomic() for unmapping" (or anything similar) is=
=20
_not_ what he wants to see.=20

=46urthermore, a large part of this text has been written by him (I'm talki=
ng=20
of a couple of weeks ago, when this patch was not part of this series - it=
=20
was on its own until Ira Weiny asked me to gather 4 patches in one only=20
series).

> This indicates the "migration" is disabled for
> !PREEMPT_RT which is not the case.

I read again how kmap_atomic() is defined. There are lots of 'if'=20
statements. Only if the code gets to __kmap_local_pfn_prot(), users can be=
=20
assured that it unconditionally calls both migrate_disable() and=20
preempt_disable().=20

> So maybe something like
>=20
>  * Unmaps an address previously mapped by kmap_atomic() and re-enables
>  * pagefaults, CPU migration (CONFIG_PREEMPT_RT) or preemption
>  * (!CONFIG_PREEMPT_RT). Mappings should be unmapped in the reverse
>=20
> will make it clear.

I'm starting to think that this level of detail is too much for users who=20
just need to understand how to use this function as well as =20
kmap_local_page().

I prefer something like the following:

+ * Unmaps an address previously mapped by kmap_atomic() and re-enables
+ * pagefaults and possibly also CPU migration and/or preemption. However,=
=20
+ * users should not count on disable of migration and/or preemption as a=20
+ * side effect of calling kmap_atomic(). Mappings must be unmapped in the=
=20
+ * reverse [...]

I'd also like to write the same paragraph for kmap_local_page().

What do you think of being less detailed and instead using the text I wrote=
=20
above?=20

Thanks,

=46abio