Message-ID: <20160818163047.GL3296@wotan.suse.de>
Date:   Thu, 18 Aug 2016 18:30:47 +0200
From:   "Luis R. Rodriguez" <mcgrof@...nel.org>
To:     Daniel Wagner <wagi@...om.org>
Cc:     "Luis R. Rodriguez" <mcgrof@...nel.org>,
        Ming Lei <ming.lei@...onical.com>,
        linux-kernel@...r.kernel.org,
        Daniel Wagner <daniel.wagner@...-carit.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Takashi Iwai <tiwai@...e.de>,
        Kees Cook <keescook@...omium.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Julia Lawall <julia.lawall@...6.fr>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Jessica Yu <jeyu@...hat.com>, Jiri Kosina <jikos@...nel.org>,
        Miroslav Benes <mbenes@...e.cz>,
        Petr Mladek <pmladek@...e.com>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH v1] firmware_class: encapsulate firmware loading status

On Wed, Aug 17, 2016 at 08:47:24AM +0200, Daniel Wagner wrote:
> Hi Luis,
> 
> On 08/10/2016 08:52 PM, Luis R. Rodriguez wrote:
> >On Wed, Aug 10, 2016 at 09:02:08AM +0200, Daniel Wagner wrote:
> >>On 10.08.2016 03:10, Luis R. Rodriguez wrote:
> >>>On Thu, Aug 04, 2016 at 02:27:16PM +0200, Daniel Wagner wrote:
> >>>>From: Daniel Wagner <daniel.wagner@...-carit.de>
> >>>I see. But in this case the code in question should never run in IRQ context?
> >>
> >>No, this code is not running in IRQ context. See below.
> >
> >OK so even for RT this is not needed then. Is that right ?
> >
> >If this is true there must be some gains of swait over old wait
> >API even if its not important for -rt, what are the selling points,
> >in summary ?
> 
> Clearly I need to improve my commit message writing. Your
> observation is correct.
> 
> The current 'state machine' uses three variables to handle the state
> and the transitions.
> 
> struct completion {
> 	unsigned int done;
> 	wait_queue_head_t wait;
> };
> 
> struct firmware_buf {
> 	...
> 	struct completion completion;
> 	unsigned long status;
> 	...
> };
> 
> Obviously, the variable 'status' holds the state. 'wait' and 'done'
> handles the synchronization. 'done' remembers how many waiters will
> be woken at max. complete_all() sets it to UMAX/2. That should be
> enough in most of the cases. 

Thanks, this helps and makes sense. How many data structures
in comparison does the new swait require ? Is it smaller ? If
so that is a nice simplification indeed, however we should make
sure we have no compromises then.

> So any future wait_for_completion() call will not block.

This I don't get, do you mean that if we have already UMAX/2
waiters on a completion and another one comes in, it will not
wait at all ?

Is this documented well ? Either way clarifying exactly what is
done here would be of huge help understanding the striking
differences between a switch to the new API.

> The patch just drops the 'done' completely because it is not
> necessary. We have a waiter queue for all those pending waiters 

So there is no limit to waiters with the new API ?

> and
> as soon the final state is reached we just wake them up. The future
> waiters will never be queued because we just check for the state
> first.

I do not follow what this means, I take it here we are talking about
possible race conditions between a wait and some work about to be
done?

> wait vs swait: The main difference between the two APIs is the
> implementation. So it is pretty simple to switch from one to the
> other. So why swait, I hear you asking. The swait implentation is
> pretty simple for the price that you can't do all the stuff what
> wait offers. As long you don't need the extra features of wait just
> go with swait.

OK so wait offers more features and its a kitchen sink of stuff,
we only require a simple wait and swait is better and more light
weight. The above number of waiters is still something I'd like
a bit clarification on.

> While the above points are nice side effect the real reason is the
> cleanup of the code and getting rid of the mutex operations.

This indeed is huge and this can better be reflected on the commit log.
In fact I wonder if its possible to do the switch without the change
to swait, and do the conversion to swait as a secondary step.

> >>>>That could lead to unbounded
> >>>>work in the IRQ context and that is a no go for -rt.
> >>>
> >>>Is the fear of the call to be used in IRQ context or the waiters to
> >>>somehow work in IRQ context somehow. The waiters were sleeping.. so
> >>>that I think leaves only the call site of the complete_all() to worry
> >>>about, but I can't see that happening in IRQ context. Please
> >>>correct me if I'm wrong.
> >>
> >>The only problem for -rt is if the complete_all() happens in IRQ
> >>context. If that happens the waker wakes up all waiters in one go (in
> >>IRQ). That leads to the 'unbounded work' which can't be preempted. There
> >>is no further restriction for -rt on waiters or wakers.
> >
> >In that case, even when -rt, this is not needed. However the compartamentalizing
> >of usermode sleep crap to usermode helper only seems worthy endeavor and I
> >wonder if we can split the work in this patch to 2, one which splits the
> >stuff, and the other one that makes then the conversion from old wait to
> >the new swait. If this is possible there are three gains:
> >
> > o makes code easier to review
> > o makes each change atomically justifiable
> 
> I can try to split the patch into two steps. Let's see how this
> works out. But I wouldn't mind if we go with this version :)

I understand -- however I have to ask as if its possible it makes
things easier to review and makes two logical changes split up. This
would in turn be easier to debug if there are issues.

> > o once you have only a conversion from old wait to new swait you can
> >   inspect the delta and try to write SmPL grammar to see if you can
> >   generalize the change, so grammar can do the change for other
> >   use cases. Of course, you'd need first to look for the IRQ context,
> >   and I wonder if that's possible. If there are however generic
> >   benefits of swait over old wait when complete_all() is used (is
> >   live patching one?) then this will be very handy.
> 
> From my attempts to figure out the execution context with SmPL I
> fear that is rather hard to achieve because you need to create a
> call graph and track the state.

OK..

> >>>>So here the
> >>>>attempt to reduce the number of complete_all() calls where possible.
> >>>
> >>>OK so this is the real motivation.
> >>
> >>Yes, this is more ore less a clean up work :)
> >>
> >>>>I have left this argument out in the commit message because I was told '-rt'
> >>>>arguments don't count for inclusion.
> >>>
> >>>Sure, but I appreciate this explanation, thanks for that !
> >>>
> >>>Can you provide a set of commits accepted upstream or on linux-next
> >>>where such conversion has been done and accepted as well elsewhere
> >>>in the kernel ?
> >>
> >>Not so far. I have started to send out patches last week. It seems most
> >>people are enjoying holiday.
> >>
> >>https://lkml.org/lkml/2016/8/4/264
> >>https://patchwork.kernel.org/project/linux-amlogic/list/?submitter=47731
> >
> >OK thanks do we have a kselftest for swait ?
> 
> No. A quick grep didn't show any test for wait either. I should
> still have some test code around for swait while hacking on it. I'll
> add it to my todo list if you think that is a worthwhile exercise.

Definitely!

  Luis

Powered by blists - more mailing lists