| Message ID | 20210215204909.3824509-1-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/gfp: Add kernel-doc for gfp_t | expand |
On Mon, Feb 15, 2021 at 08:49:09PM +0000, Matthew Wilcox (Oracle) wrote: > The generated html will link to the definition of the gfp_t automatically > once we define it. Move the one-paragraph overview of GFP flags from the > documentation directory into gfp.h and pull gfp.h into the documentation. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> Acked-by: Mike Rapoport <rppt@linux.ibm.com> > --- > Documentation/core-api/mm-api.rst | 7 ++----- > include/linux/gfp.h | 11 +++++++++++ > 2 files changed, 13 insertions(+), 5 deletions(-) > > diff --git a/Documentation/core-api/mm-api.rst b/Documentation/core-api/mm-api.rst > index 2adffb3f7914..201b5423303b 100644 > --- a/Documentation/core-api/mm-api.rst > +++ b/Documentation/core-api/mm-api.rst > @@ -19,11 +19,8 @@ User Space Memory Access > Memory Allocation Controls > ========================== > > -Functions which need to allocate memory often use GFP flags to express > -how that memory should be allocated. The GFP acronym stands for "get > -free pages", the underlying memory allocation function. Not every GFP > -flag is allowed to every function which may allocate memory. Most > -users will want to use a plain ``GFP_KERNEL``. > +.. kernel-doc:: include/linux/gfp.h > + :internal: > > .. kernel-doc:: include/linux/gfp.h > :doc: Page mobility and placement hints > diff --git a/include/linux/gfp.h b/include/linux/gfp.h > index 52cd415b436c..ecd1b5d27936 100644 > --- a/include/linux/gfp.h > +++ b/include/linux/gfp.h > @@ -8,6 +8,17 @@ > #include <linux/linkage.h> > #include <linux/topology.h> > > +/** > + * typedef gfp_t - Memory allocation flags. > + * > + * GFP flags are commonly used throughout Linux to indicate how memory > + * should be allocated. The GFP acronym stands for "get free pages", > + * the underlying memory allocation function. Not every GFP flag is > + * supported by every function which may allocate memory. Most users > + * will want to use a plain ``GFP_KERNEL``. > + */ > +typedef unsigned int __bitwise gfp_t; // repeated here for kernel-doc > + > struct vm_area_struct; > > /* > -- > 2.29.2 >
On Mon, Feb 15, 2021 at 08:49:09PM +0000, Matthew Wilcox (Oracle) wrote: > The generated html will link to the definition of the gfp_t automatically > once we define it. Move the one-paragraph overview of GFP flags from the > documentation directory into gfp.h and pull gfp.h into the documentation. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> This patch causes a clang warning in basically every file on linux-next now: include/linux/gfp.h:20:32: warning: redefinition of typedef 'gfp_t' is a C11 feature [-Wtypedef-redefinition] typedef unsigned int __bitwise gfp_t; // repeated here for kernel-doc ^ include/linux/types.h:148:32: note: previous definition is here typedef unsigned int __bitwise gfp_t; ^ 1 warning generated. Cheers, Nathan
On Fri, Feb 19, 2021 at 12:55:09PM -0700, Nathan Chancellor wrote: > On Mon, Feb 15, 2021 at 08:49:09PM +0000, Matthew Wilcox (Oracle) wrote: > > The generated html will link to the definition of the gfp_t automatically > > once we define it. Move the one-paragraph overview of GFP flags from the > > documentation directory into gfp.h and pull gfp.h into the documentation. > > > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > > This patch causes a clang warning in basically every file on linux-next > now: > > include/linux/gfp.h:20:32: warning: redefinition of typedef 'gfp_t' is a C11 feature [-Wtypedef-redefinition] Seems like it's also a gnu89 feature.
On Fri, Feb 19, 2021 at 12:55 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Fri, Feb 19, 2021 at 12:55:09PM -0700, Nathan Chancellor wrote: > > On Mon, Feb 15, 2021 at 08:49:09PM +0000, Matthew Wilcox (Oracle) wrote: > > > The generated html will link to the definition of the gfp_t automatically > > > once we define it. Move the one-paragraph overview of GFP flags from the > > > documentation directory into gfp.h and pull gfp.h into the documentation. > > > > > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > > > > This patch causes a clang warning in basically every file on linux-next > > now: > > > > include/linux/gfp.h:20:32: warning: redefinition of typedef 'gfp_t' is a C11 feature [-Wtypedef-redefinition] > > Seems like it's also a gnu89 feature. I'm not sure a lack of a warning is an intentional feature, and would bet that behavior is not documented. That said, I'm fine disabling this warning; there's a separate error for redefining a typedef to a different underlying type. That's what's useful IMO, this one really is not. This warning doesn't really provide any value to us in the kernel; I would guess the intent was to be helpful to code expected to be portable across different -std=*, but that's not the case for the Linux kernel. (It's also trivial to change this in Clang, but we'd have to disable this warning for older supported of clang anyways). diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index cee107096947..63529a43e797 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -2381,7 +2381,7 @@ void Sema::MergeTypedefNameDecl(Scope *S, TypedefNameDecl *New, } // Modules always permit redefinition of typedefs, as does C11. - if (getLangOpts().Modules || getLangOpts().C11) + if (getLangOpts().Modules || getLangOpts().C11 || getLangOpts().GNUMode) return; // If we have a redefinition of a typedef in C, emit a warning. This warning
On Fri, Feb 19, 2021 at 10:45 PM 'Nick Desaulniers' via Clang Built Linux <clang-built-linux@googlegroups.com> wrote: > > That said, I'm fine disabling this warning; there's a separate error > for redefining a typedef to a different underlying type. That's > what's useful IMO, this one really is not. > > This warning doesn't really provide any value to us in the kernel; I > would guess the intent was to be helpful to code expected to be > portable across different -std=* It seems it would also be useful to sport unintended cases, e.g.: - Collisions on short identifiers (that by chance typedef to the same type). - Copy-pasting and forgetting to remove the original definition (i.e. it should have be cut-pasting instead). - Double inclusion of headers (with missing or broken #ifdef guards). Those may be providing value in the kernel. In particular, if we don't see any warning at the moment, it means those cases are not happening now anywhere, so we would be weakening things. Having said that, I don't see the original patch, so perhaps I am missing something. Cheers, Miguel
On Fri, Feb 19, 2021 at 2:15 PM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > On Fri, Feb 19, 2021 at 10:45 PM 'Nick Desaulniers' via Clang Built > Linux <clang-built-linux@googlegroups.com> wrote: > > > > That said, I'm fine disabling this warning; there's a separate error > > for redefining a typedef to a different underlying type. That's > > what's useful IMO, this one really is not. > > > > This warning doesn't really provide any value to us in the kernel; I > > would guess the intent was to be helpful to code expected to be > > portable across different -std=* > > It seems it would also be useful to sport unintended cases, e.g.: > > - Collisions on short identifiers (that by chance typedef to the same type). > - Copy-pasting and forgetting to remove the original definition > (i.e. it should have be cut-pasting instead). > - Double inclusion of headers (with missing or broken #ifdef guards). (There is a separate warning flag for broken header guards, -Wheader-guard: https://github.com/ClangBuiltLinux/linux/issues?q=is%3Aissue+label%3A-Wheader-guard+is%3Aclosed) What happens should the kernel move to gnu11, as discussed once GCC 5.1+ becomes the minimum supported version for all arches? https://lore.kernel.org/lkml/CAHk-=whnKkj5CSbj-uG_MVVUsPZ6ppd_MFhZf_kpXDkh2MAVRA@mail.gmail.com/ Then the warning will not fire, since it's only really meant to help C code be portable between -std=c11. (Another change to clang could be to move this flag into the -Wpedantic group, which is only really meant for from guarding against the use of non-ISO C functionality, but that still would require disabling the warning for older but supported versions of clang). > > Those may be providing value in the kernel. In particular, if we don't > see any warning at the moment, it means those cases are not happening > now anywhere, so we would be weakening things. > > Having said that, I don't see the original patch, so perhaps I am > missing something. https://lore.kernel.org/linux-mm/20210215204909.3824509-1-willy@infradead.org/
On Fri, Feb 19, 2021 at 11:50 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > (There is a separate warning flag for broken header guards, > -Wheader-guard: > https://github.com/ClangBuiltLinux/linux/issues?q=is%3Aissue+label%3A-Wheader-guard+is%3Aclosed) Yeah, this would still help cases with headers without guards (not really common, but...). > What happens should the kernel move to gnu11, as discussed once GCC > 5.1+ becomes the minimum supported version for all arches? > https://lore.kernel.org/lkml/CAHk-=whnKkj5CSbj-uG_MVVUsPZ6ppd_MFhZf_kpXDkh2MAVRA@mail.gmail.com/ > > Then the warning will not fire, since it's only really meant to help C > code be portable between -std=c11. I think it is a fine warning to keep in the compiler nevertheless since you already have it, even if disabled by default or only in -Wextra or something like that -- e.g. I would use it in my own projects since I never intend to redefine a typedef. > https://lore.kernel.org/linux-mm/20210215204909.3824509-1-willy@infradead.org/ Thanks for the link! Cheers, Miguel
On Fri, Feb 19, 2021 at 12:55 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Fri, Feb 19, 2021 at 12:55:09PM -0700, Nathan Chancellor wrote: > > On Mon, Feb 15, 2021 at 08:49:09PM +0000, Matthew Wilcox (Oracle) wrote: > > > The generated html will link to the definition of the gfp_t automatically > > > once we define it. Move the one-paragraph overview of GFP flags from the > > > documentation directory into gfp.h and pull gfp.h into the documentation. > > > > > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > > > > This patch causes a clang warning in basically every file on linux-next > > now: > > > > include/linux/gfp.h:20:32: warning: redefinition of typedef 'gfp_t' is a C11 feature [-Wtypedef-redefinition] > > Seems like it's also a gnu89 feature. Is there a preprocessor define when using kernel doc that you could guard the redefinition with? See include/linux/phylink.h which uses: #if 0 /* For kernel-doc purposes only. */ to guard definitions for kernel-doc.
On Mon, Feb 22, 2021 at 09:04:40AM -0800, Nick Desaulniers wrote:
> #if 0 /* For kernel-doc purposes only. */
Yeah, I did that already over the weekend. Not sure if akpm has published
a new mmotm that includes it yet.
On Mon, Feb 22, 2021 at 9:16 AM Matthew Wilcox <willy@infradead.org> wrote: > > On Mon, Feb 22, 2021 at 09:04:40AM -0800, Nick Desaulniers wrote: > > #if 0 /* For kernel-doc purposes only. */ > > Yeah, I did that already over the weekend. Not sure if akpm has published > a new mmotm that includes it yet. ah, sorry I missed it then. Appreciated!
diff --git a/Documentation/core-api/mm-api.rst b/Documentation/core-api/mm-api.rst index 2adffb3f7914..201b5423303b 100644 --- a/Documentation/core-api/mm-api.rst +++ b/Documentation/core-api/mm-api.rst @@ -19,11 +19,8 @@ User Space Memory Access Memory Allocation Controls ========================== -Functions which need to allocate memory often use GFP flags to express -how that memory should be allocated. The GFP acronym stands for "get -free pages", the underlying memory allocation function. Not every GFP -flag is allowed to every function which may allocate memory. Most -users will want to use a plain ``GFP_KERNEL``. +.. kernel-doc:: include/linux/gfp.h + :internal: .. kernel-doc:: include/linux/gfp.h :doc: Page mobility and placement hints diff --git a/include/linux/gfp.h b/include/linux/gfp.h index 52cd415b436c..ecd1b5d27936 100644 --- a/include/linux/gfp.h +++ b/include/linux/gfp.h @@ -8,6 +8,17 @@ #include <linux/linkage.h> #include <linux/topology.h> +/** + * typedef gfp_t - Memory allocation flags. + * + * GFP flags are commonly used throughout Linux to indicate how memory + * should be allocated. The GFP acronym stands for "get free pages", + * the underlying memory allocation function. Not every GFP flag is + * supported by every function which may allocate memory. Most users + * will want to use a plain ``GFP_KERNEL``. + */ +typedef unsigned int __bitwise gfp_t; // repeated here for kernel-doc + struct vm_area_struct; /*
The generated html will link to the definition of the gfp_t automatically once we define it. Move the one-paragraph overview of GFP flags from the documentation directory into gfp.h and pull gfp.h into the documentation. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- Documentation/core-api/mm-api.rst | 7 ++----- include/linux/gfp.h | 11 +++++++++++ 2 files changed, 13 insertions(+), 5 deletions(-)