Message-ID: <20201014175143.3d594341@coco.lan>
Date:   Wed, 14 Oct 2020 18:04:59 +0200
From:   Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Linux Doc Mailing List <linux-doc@...r.kernel.org>,
        Jonathan Corbet <corbet@....net>,
        Boqun Feng <boqun.feng@...il.com>,
        Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 04/24] docs: lockdep-design: fix some warning issues

Em Tue, 13 Oct 2020 16:02:50 +0100
Matthew Wilcox <willy@...radead.org> escreveu:

> On Tue, Oct 13, 2020 at 04:09:41PM +0200, Peter Zijlstra wrote:
> > On Tue, Oct 13, 2020 at 02:11:16PM +0100, Matthew Wilcox wrote:
> > > On Tue, Oct 13, 2020 at 02:52:06PM +0200, Peter Zijlstra wrote:
> > > > On Tue, Oct 13, 2020 at 02:14:31PM +0200, Mauro Carvalho Chehab wrote:
> > > > > +   =====  ===================================================
> > > > > +   ``.``  acquired while irqs disabled and not in irq context
> > > > > +   ``-``  acquired in irq context
> > > > > +   ``+``  acquired with irqs enabled
> > > > > +   ``?``  acquired in irq context with irqs enabled.
> > > > > +   =====  ===================================================
> > > >
> > > > NAK!
> > > 
> > > You're seriously suggesting that:
> > > 
> > > -   ===  ===================================================
> > > -   '.'  acquired while irqs disabled and not in irq context
> > > -   '-'  acquired in irq context
> > > -   '+'  acquired with irqs enabled
> > > -   '?'  acquired in irq context with irqs enabled.
> > > -   ===  ===================================================
> > > +   =====  ===================================================
> > > +   ``.``  acquired while irqs disabled and not in irq context
> > > +   ``-``  acquired in irq context
> > > +   ``+``  acquired with irqs enabled
> > > +   ``?``  acquired in irq context with irqs enabled.
> > > +   =====  ===================================================
> > > 
> > > this change makes the lockdep docs less readable?
> > 
> > Definitely makes it harder to read for me. My C trained eyes go WTF at
> > seeing it, which breaks the flow. ',' is a regular single character
> > constant, '','' a syntax error.
> 
> OK, that's fair.  'a' is definitely a character constant.  Perhaps
> the automarkup script can take care of this for us?  We'd have to
> be careful not to catch anything we shouldn't've [1], but I'm sure
> there's a regex for it.  Something like "\<'.'\>", perhaps?

I guess that this regex could work:

	/\b\'\S\'\b/ 

would get this very specific case, or maybe even:
	/\b\'\S+\'\b/

in order to get things like 'foo'.

Adding support for something like this at 
Documentation/sphinx/automarkup.py should be trivial. However,
checking if this won't be doing anything wrong with the other existing
files can be painful.

Yet, there are 3 issues related to '.' character usage.

See, the first table is:

   ===  ===================================================
   '.'  acquired while irqs disabled and not in irq context
   '-'  acquired in irq context
   '+'  acquired with irqs enabled
   '?'  acquired in irq context with irqs enabled.
   ===  ===================================================

There, it uses '.' in order to indicate the dot character and so on.

The second table uses a different notation:

   +--------------+-------------+--------------+
   |              | irq enabled | irq disabled |
   +--------------+-------------+--------------+
   | ever in irq  |      ?      |       -      |
   +--------------+-------------+--------------+
   | never in irq |      +      |       .      |
   +--------------+-------------+--------------+

which uses just a question mark without aphostrophes, instead of
'?' (and the same for the other symbols).

The text describing them returns back to the notation used at the
first table:

	"The character '-' suggests irq is disabled because if otherwise the
	 charactor '?' would have been shown instead. Similar deduction can be
	 applied for '+' too."

-

The above has actually 3 separate problems:

1) This problem has nothing to do with Sphinx notation. The notation
   is not coherent: It should use either ., ``.`` or '.' everywhere.  

2) This is Sphinx-specific: a single minus or a single plus character
indicates a list. On both cases, this is actually replaced by an UTF-8
bullet character: '•'.

3) This is a minor issue:

   using '.' will produce an html table that will display, using
   a normal font, as '.', while ``.`` would use a monospaced 
   font and won't display the apostrophes. IMO, at the html output, 
   it would be better to just a dot without apostrophes, as the
   text of the dmesg output doesn't have apostrophes either:

     "When locking rules are violated, these usage bits are presented in the
      locking error messages, inside curlies, with a total of 2 * n STATEs bits.
      A contrived example::

        modprobe/2287 is trying to acquire lock:
         (&sio_locks[i].lock){-.-.}, at: [<c02867fd>] mutex_lock+0x21/0x24

        but task is already holding lock:
         (&sio_locks[i].lock){-.-.}, at: [<c02867fd>] mutex_lock+0x21/0x24"

   Yet, we could live without addressing this one.

> [1] I'm quite proud of that one.
> 
> > > It's not the markup that makes the lockdep documentation hard to
> > > understand.
> > 
> > I'm not sure what you're alluding to, the subject just isn't easy to
> > begin with.
> 
> Absolutely.  The problem is (similar to most Linux documentation)
> the document doesn't know who its audience is.  It mixes internal
> implementation details of lockdep with what people need to know who
> are just trying to understand what a lockdep splat means.  I don't
> have time to restructure it right now though.

Yeah, that is one of the the main issues with this. This specific 
section of the file describes something that a sysadmin or a Kernel
newbie may see on his system. He'll likely seek the web for some
documentation about that, in order to try to understand it.

If someone is willing to do that, it will get this:

	https://www.kernel.org/doc/html/latest/locking/lockdep-design.html#state

Where it presents a plain wrong table that it would look like this:

   +--------------+-------------+--------------+
   |              | irq enabled | irq disabled |
   +--------------+-------------+--------------+
   | ever in irq  |      ?      |       •      |
   +--------------+-------------+--------------+
   | never in irq |      •      |       .      |
   +--------------+-------------+--------------+

If you prefer, I can send a new version of this patch using this at
the second table:

   +--------------+-------------+--------------+
   |              | irq enabled | irq disabled |
   +--------------+-------------+--------------+
   | ever in irq  |     '?'     |      '-'     |
   +--------------+-------------+--------------+
   | never in irq |     '+'     |      '.'     |
   +--------------+-------------+--------------+

and keep using '.' at the other parts of the document.

This should solve the Sphinx issue, although the Sphinx output
won't be using a monospaced font.

Thanks,
Mauro

Powered by blists - more mailing lists