All of lore.kernel.org
 help / color / mirror / Atom feed
From: Matthew Wilcox <willy@infradead.org>
To: linux-doc@vger.kernel.org
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Subject: Hawkmoth & kernel-doc
Date: Thu, 22 Jul 2021 20:15:34 +0100	[thread overview]
Message-ID: <YPnD1vkOTDhktnei@casper.infradead.org> (raw)

I really dislike the javadoc style, so at Jani's urging I gave Hawkmoth
a try.  It's a little fiddly and a little buggy, but I think it
encourages better-written documentation.

I chose mm_inline.h as my victim.  It isn't currently included in the
documentation, and it only has three kernel-doc comments in it to
convert.

I'm approximating what firefox displays here ...

int page_is_file_lru(struct page *page)

  Should the page be on a file LRU or anon LRU?

    Parameters:  * page – The page to test.

    We would like to get this info without a page flag, but the state
    needs to survive until the page is last deleted from the LRU, which
    could be as far down as __page_cache_release.

    Returns:

             An integer (not a boolean!) used to sort a page onto the right
             LRU list and to account folios correctly.

             * 1 if @page is a regular filesystem backed page cache page or a lazily freed anonymous page (e.g. via MADV_FREE).
             * 0 if @page is a normal anonymous page, a tmpfs page or otherwise ram or swap backed page.

That's not _bad_.

1. 'Parameters' is indented further than kernel-doc does (2 levels of
   indent, rather than 0)

2. Output:
<dd><p>Should the page be on a file LRU or anon LRU?</p>
<dl class="field-list simple">
<dt class="field-odd">Parameters</dt>
<dd class="field-odd"><ul class="simple">
<li><p><strong>page</strong> – The page to test.</p></li>
</ul>
</dd>
</dl>
having gone to the trouble of using a <dl>, I feel that each parameter
should be a <dt> and its definition a <dd>.  That seems to be what
kernel-doc is doing.

3. kernel-doc includes the type of the parameter in the definition,
   while hawkmoth drops it.  I'm ambivalent about which is better.

4. It seems that the indentation of the 'Returns:' paragraph is a bit
   too deep, but it did get the list correct.

5. The second and third functions documented by hawkmoth lose their
   parameter in the initial heading:


int __clear_page_lru_flags()

    Clear page lru flags before releasing a page.

    Parameters  * page – The page that was on lru and now has a zero reference

That first line really should have a 'struct page *page' in it.

6. There's also a warning from the build:

/home/willy/kernel/linux/Documentation/core-api/mm-api.rst:101: WARNING: /home/willy/kernel/linux/include/linux/mm_inline.h:5: 'linux/huge_mm.h' file not found

Looks like I can probably pass arbitrary flags to clang to get it to
find that file, but I'm not sure whether I should.

Here's the diff against current Linus tree that I'm working with:

diff --git a/Documentation/conf.py b/Documentation/conf.py
index 7d92ec3e5b6e..13fcce3ec18d 100644
--- a/Documentation/conf.py
+++ b/Documentation/conf.py
@@ -39,7 +39,7 @@ needs_sphinx = '1.7'
 extensions = ['kerneldoc', 'rstFlatTable', 'kernel_include',
               'kfigure', 'sphinx.ext.ifconfig', 'automarkup',
               'maintainers_include', 'sphinx.ext.autosectionlabel',
-              'kernel_abi', 'kernel_feat']
+              'kernel_abi', 'kernel_feat', 'hawkmoth']
 
 if major >= 3:
     if (major > 3) or (minor > 0 or patch >= 2):
@@ -104,6 +104,7 @@ if major >= 3:
 else:
     extensions.append('cdomain')
 
+cautodoc_root = os.path.abspath('..')
 # Ensure that autosectionlabel will produce unique names
 autosectionlabel_prefix_document = True
 autosectionlabel_maxdepth = 2
diff --git a/Documentation/core-api/mm-api.rst b/Documentation/core-api/mm-api.rst
index adabb5c5798a..b724e29772b0 100644
--- a/Documentation/core-api/mm-api.rst
+++ b/Documentation/core-api/mm-api.rst
@@ -98,4 +98,5 @@ More Memory Management Functions
    :internal:
 .. kernel-doc:: include/linux/mm.h
    :internal:
+.. c:autodoc:: include/linux/mm_inline.h
 .. kernel-doc:: include/linux/mmzone.h
diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
index 355ea1ee32bd..7db1193a0d94 100644
--- a/include/linux/mm_inline.h
+++ b/include/linux/mm_inline.h
@@ -6,18 +6,21 @@
 #include <linux/swap.h>
 
 /**
- * page_is_file_lru - should the page be on a file LRU or anon LRU?
- * @page: the page to test
+ * Should the page be on a file LRU or anon LRU?
  *
- * Returns 1 if @page is a regular filesystem backed page cache page or a lazily
- * freed anonymous page (e.g. via MADV_FREE).  Returns 0 if @page is a normal
- * anonymous page, a tmpfs page or otherwise ram or swap backed page.  Used by
- * functions that manipulate the LRU lists, to sort a page onto the right LRU
- * list.
+ * :param page: The page to test.
  *
  * We would like to get this info without a page flag, but the state
  * needs to survive until the page is last deleted from the LRU, which
  * could be as far down as __page_cache_release.
+ *
+ * :return: An integer (not a boolean!) used to sort a page onto the right
+ *   LRU list and to account folios correctly.
+ *
+ *   - 1 if @page is a regular filesystem backed page cache page or a lazily
+ *     freed anonymous page (e.g. via MADV_FREE).
+ *   - 0 if @page is a normal anonymous page, a tmpfs page or otherwise ram or
+ *     swap backed page.
  */
 static inline int page_is_file_lru(struct page *page)
 {
@@ -39,8 +42,9 @@ static __always_inline void update_lru_size(struct lruvec *lruvec,
 }
 
 /**
- * __clear_page_lru_flags - clear page lru flags before releasing a page
- * @page: the page that was on lru and now has a zero reference
+ * Clear page lru flags before releasing a page.
+ *
+ * :param page: The page that was on lru and now has a zero reference
  */
 static __always_inline void __clear_page_lru_flags(struct page *page)
 {
@@ -57,11 +61,12 @@ static __always_inline void __clear_page_lru_flags(struct page *page)
 }
 
 /**
- * page_lru - which LRU list should a page be on?
- * @page: the page to test
+ * Which LRU list should a page be on?
+ *
+ * :param page: The page to test
  *
- * Returns the LRU list a page should be on, as an index
- * into the array of LRU lists.
+ * :return: The LRU list a page should be on, as an index into the array
+ *          of LRU lists.
  */
 static __always_inline enum lru_list page_lru(struct page *page)
 {


             reply	other threads:[~2021-07-22 19:16 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-22 19:15 Matthew Wilcox [this message]
2021-08-09 17:57 ` Hawkmoth & kernel-doc Matthew Wilcox
2021-08-11 15:26 ` Jani Nikula

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YPnD1vkOTDhktnei@casper.infradead.org \
    --to=willy@infradead.org \
    --cc=jani.nikula@linux.intel.com \
    --cc=linux-doc@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.