lkml.org 
[lkml]   [2016]   [Aug]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v1] firmware_class: encapsulate firmware loading status
Date
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

\
 
 \ /
  Last update: 2016-09-17 09:57    [W:0.158 / U:0.092 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site