linux-kernel - Re: [PATCH v2 1/4] mm/highmem: Fix kernel-doc warnings in highmem*.h

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 26 Apr 2022 11:43:03 +0200
From:   "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:     Ira Weiny <ira.weiny@...el.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Catalin Marinas <catalin.marinas@....com>,
        "Matthew Wilcox (Oracle)" <willy@...radead.org>,
        Will Deacon <will@...nel.org>,
        Peter Collingbourne <pcc@...gle.com>,
        Vlastimil Babka <vbabka@...e.cz>, linux-kernel@...r.kernel.org,
        Jonathan Corbet <corbet@....net>, linux-doc@...r.kernel.org,
        outreachy@...ts.linux.dev,
        "Acked-by : Mike Rapoport" <rppt@...ux.ibm.com>
Subject: Re: [PATCH v2 1/4] mm/highmem: Fix kernel-doc warnings in highmem*.h

On martedì 26 aprile 2022 09:01:32 CEST Sebastian Andrzej Siewior wrote:
> On 2022-04-25 18:23:57 [+0200], Fabio M. De Francesco wrote:
> > index a77be5630209..aa22daeed617 100644
> > --- a/include/linux/highmem-internal.h
> > +++ b/include/linux/highmem-internal.h
> > @@ -236,9 +236,17 @@ static inline unsigned long totalhigh_pages(void) 
{ return 0UL; }
> >  
> >  #endif /* CONFIG_HIGHMEM */
> >  
> > -/*
> > - * Prevent people trying to call kunmap_atomic() as if it were 
kunmap()
> > - * kunmap_atomic() should get the return value of kmap_atomic, not the 
page.
> > +/**
> > + * kunmap_atomic - Unmap the virtual address mapped by kmap_atomic()
> > + * @__addr:       Virtual address to be unmapped
> > + *
> > + * Unmaps an address previously mapped by kmap_atomic() and re-enables
> > + * pagefaults and preemption. Mappings should be unmapped in the 
reverse
> 
> You mind adding "Deprecated!" like kmap_atomic() has? 

I might add "Deprecated!", however Ira Weiny asked me to rephrase an 
earlier version of one of the patch which is is this series. I wrote that 
"The use of kmap_atomic() is deprecated in favor of kmap_local_page()." and 
Ira replied "I'm not sure deprecated is the right word. [] This series 
should end up indicating the desire to stop growing kmap() and
kmap_atomic() call sites and that their deprecation is on the horizon.".

What Ira suggested is exactly what I'm doing in v2. 

@Ira: what about adding "Deprecated!" for consistency with kmap_atomic() 
kdoc?

> The part about
> disabling/ enabling preemption is true for !PREEMPT_RT.

To me it looks that this is not what Thomas Gleixner wrote in the cover 
letter of his series ("[patch V2 00/18] mm/highmem: Preemptible variant of 
kmap_atomic & friends") at 
https://lore.kernel.org/lkml/20201029221806.189523375@linutronix.de/

For your convenience:

"[] there is not a real reason anymore to confine migration disabling to 
RT. [] Removing the RT dependency from migrate_disable/enable()".

Is there anything I'm still missing?

> The part that
> worries me is that people use it and rely on disabled preemption like
> some did in the past. 

This is something I'd prefer to hear also from other developers who are 
CC'ed for this patch :) 

> I've been told this API is about to be removed (or so I have been told)
> so I hope that it will be gone soon ;)
> 
> > + * order that they were mapped. See kmap_local_page() for details.
> > + * @__addr can be any address within the mapped page, so there is no 
need
> > + * to subtract any offset that has been added. In contrast to 
kunmap(),
> > + * this function takes the address returned from kmap_atomic(), not 
the
> > + * page passed to it. The compiler will warn you if you pass the page.
> >   */
> >  #define kunmap_atomic(__addr)					
\
> >  do {								
\
> 
> Sebastian
> 

Thanks for your review,

Fabio