linux-kernel - Re: [PATCH RFC] doc: rcu: remove obsolete (non-)requirement about disabling preemption

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:   Mon, 15 Oct 2018 13:15:56 -0700
From:   Joel Fernandes <joel@...lfernandes.org>
To:     "Paul E. McKenney" <paulmck@...ux.ibm.com>
Cc:     Nikolay Borisov <nborisov@...e.com>, linux-kernel@...r.kernel.org,
        Jonathan Corbet <corbet@....net>,
        Josh Triplett <josh@...htriplett.org>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        linux-doc@...r.kernel.org,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [PATCH RFC] doc: rcu: remove obsolete (non-)requirement about
 disabling preemption

On Mon, Oct 15, 2018 at 12:54:26PM -0700, Paul E. McKenney wrote:
[...]
> > > In any case, please don't spin for milliseconds with preemption disabled.
> > > The real-time guys are unlikely to be happy with you if you do this!
> > 
> > Well just to clarify, I was just running Oleg's test which did this. This
> > test was mentioned in the original documentation that I deleted. Ofcourse I
> > would not dare do such a thing in production code :-D. I guess to Oleg's
> > defense, he did it to very that synchronize_rcu() was not blocked on
> > preempt-disable sections which was a different test.
> 
> Understood!  Just pointing out that RCU's tolerating a given action does
> not necessarily mean that it is a good idea to take that action.  ;-)

Makes sense :-) thanks.

> > > > > +		pr_crit("SPIN done!\n");
> > > > > +		preempt_enable();
> > > > > +		break;
> > > > > +	case 777:
> > > > > +		pr_crit("SYNC start\n");
> > > > > +		synchronize_rcu();
> > > > > +		pr_crit("SYNC done!\n");
> > > > 
> > > > But you are using the console printing infrastructure which is rather
> > > > heavyweight. Try replacing pr_* calls with trace_printk so that you
> > > > write to the lock-free ring buffer, this will reduce the noise from the
> > > > heavy console printing infrastructure.
> > > 
> > > And this might be a problem as well.
> > 
> > This was not the issue (or atleast not fully the issue) since I saw the same
> > thing with trace_printk. It was exactly what you said - which is the
> > excessively long preempt disabled times.
> 
> One approach would be to apply this patch against (say) v4.18, which
> does not have consolidated grace periods.  You might then be able to
> tell if the pr_crit() calls make any difference.

I could do that, yeah. But since the original problem went away due to
disabling preempts for a short while, I will move on and continue to focus on
updating other parts of the documenation. Just to mention I
brought this up because I thought its better to do that than not to, just
incase there is any lurking issue with the consolidation. Sorry if that ended
up with me being noisy.

Just curious, while I am going through the documentation, is there anything
in particular that particularly sticks out to you that needs updating? I
think I am around 50% there with the last several rounds of doc patches but I
have lot more to go through. "Just keep doing what you're doing" is also a
perfectly valid answer ;-)

thanks,

 - Joel