Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1464357 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v1] firmware_class: encapsulate firmware loading status Daniel Wagner <wagi@monom.org> - 2016-08-17 09:00 +0200
    Re: [PATCH v1] firmware_class: encapsulate firmware loading status "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-08-19 03:40 +0200
      Re: [PATCH v1] firmware_class: encapsulate firmware loading status Daniel Wagner <wagi@monom.org> - 2016-08-19 03:40 +0200

#1464357 — Re: [PATCH v1] firmware_class: encapsulate firmware loading status

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

[toc] | [next] | [standalone]

#1465807

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

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

[toc] | [prev] | [next] | [standalone]

#1465825

On 18.08.2016 18:30, Luis R. Rodriguez wrote:
> On Wed, Aug 17, 2016 at 08:47:24AM +0200, Daniel Wagner wrote:
>> On 08/10/2016 08:52 PM, Luis R. Rodriguez wrote:
>> 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.

Yes we save one 'unsigned int', that is the done member of struct
completion. For an earlier version of this patch I did check the size
changes. While we save a little on the data section, the code section
increased slightly, IIRC it was around 60 bytes. Will do another
measurement.

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

Sorry, I think I just confused you here with a implementation detail.
Whenever wait_for_completion() is woken it checks if done > 0. Then it
will decrement the counter. complete() increases the counter and then
wakes the waiter. Basically it is comparable with semamphore put and get
operation. complete_all() just sets done to max almost infinite value :)

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

Obviously, there is Documentation/scheduler/completion.txt but the small
detail on UMAX/2 is not mentioned. I don't think it was considered to be
a real problem. I guess before you run into the problem of waking 2
billion threads you see other scaling issues first.

Note this has nothing to do with wait vs swait.

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

Correct, the limit is gone, though I don't expect that there are so many
firmware user helper waiting that we hit the UMAX/2 limit ever.

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

Let me reword that. I was not really concerned about race condition
here. I was just trying to point out that we just check for the condition.

Either we have reached FW_STATUS_{DONE|ABORTED} and just continue or we
put the thread to sleep and wait for the wake call. Because we check for
the a single condition (status == FW_STATUS_{DONE|ABORTED} in
swait_event_interruptable_timeout() we don't need any addition
synchronization. Come to think about it, that is why the mutex can be
removed.

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

Yes, that summarized it pretty good.

> The above number of waiters is still something I'd like
> a bit clarification on.

As I understand the firmware loader helper userland API there is only
one waiter.

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

Not sure about it because 'status' and the operation of completion need
to be synchronized. I'll give it a try just haven't had time yet. It is
not about wait or swait, it's about completion vs s/wait.

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

Sure, I completely understand. BTW, I just updated the patch and avoided
the moving of the loading_timeout. Now it doesn't contain any hard to
read section anymore.

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

I know you have a far better understanding. We need to discuss this over
a beer :)

cheers,
daniel

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web