Re: [PATCH v1] firmware_class: encapsulate firmware loading status

[Daniel Wagner <wagi@monom.org>]

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@bmw-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. So any future wait_for_completion() call will not block.

The patch just drops the 'done' completely because it is not necessary. 
We have a waiter queue for all those pending waiters 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.

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.

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

>>>> 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 :)

>  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.

>>>> 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.

cheers,
daniel