[PATCH] staging: greybus: operation: add generic timeout support

Johan Hovold johan at kernel.org
Wed Feb 8 14:16:36 UTC 2017


On Wed, Feb 08, 2017 at 02:05:15PM +0000, Bryan O'Donoghue wrote:
> On 08/02/17 11:55, Johan Hovold wrote:
> > On Wed, Feb 08, 2017 at 11:43:57AM +0000, Bryan O'Donoghue wrote:
> >> On 08/02/17 09:43, Johan Hovold wrote:
> >>> On Tue, Feb 07, 2017 at 02:31:04PM +0000, Bryan O'Donoghue wrote:
> >>>> On 07/02/17 14:19, Johan Hovold wrote:
> >>>>> On Mon, Jan 23, 2017 at 01:04:14PM +0100, Johan Hovold wrote:
> >>>>>> Add a struct timer_list to struct gb_operation and use that to implement
> >>>>>> generic operation timeouts.
> >>>>>>
> >>>>>> This simplifies the synchronous operation handling somewhat while also
> >>>>>> providing a generic timeout mechanism that drivers can use for
> >>>>>> asynchronous operations.
> >>>>>>
> >>>>>> Signed-off-by: Johan Hovold <johan at kernel.org>
> >>>>>
> >>>>> Greg,
> >>>>>
> >>>>> I believe you can apply this one now. This is the right way to implement
> >>>>> operation timeouts, and it is independent of Bryan's patches converting
> >>>>> loopback to use the new interface, which can be applied on top when they
> >>>>> are ready.
> > 
> >>> My approach using a single timer which either times out or is cancelled
> >>> upon completion is about as efficient as this can get, and therefore
> >>> also allows the current synchronous-operation implementation to be built
> >>> on top of this generic mechanism.
> >>
> >> I'm just wondering what impact it has instead of
> >> wait_event_interruptible() more/less overhead (I suspect more) - and I
> >> reckoned you'd not be on for that change - so only made a change on the
> >> asynchronous path.
> > 
> > Yes, your approach with unconditional scheduling of a work struct for
> > every operation was needlessly inefficient and I did not want that for
> > the synchronous path as it would definitely be a regression (even if we
> > would have gone with your patches as an intermediate step for the
> > asynchronous case).
> 
> Yes, I'm wondering what impact switching away from wait_for_completion*
> in favour of add/cancel timer has for every operation.
> 
> It's probably irrelevant.

I think so, yes. There is still a timer hidden in
wait_for_completion_timeout as mentioned below.

> > My approach does not suffer from such overhead so can therefore be used
> > as a generic mechanism (there's exactly one timer involved whether we
> > use wait_event_interruptible_timer or not).
> 
> Hmm yes, I was surprised that the first feedback from you on this was an
> alternative patch but, since you feel strongly about doing it this way,
> it's fine with me.

Why would we go with a suboptimal approach, which in its present form
still had issues that were unresolved?

I told you from the start (August?) that I did not like the deferred-
work approach and that a proper solution was planned for v2. Project
got cancelled, but I now spent some time working out an implementation
instead of spending more time on your patch.

> Acked-by: Bryan O'Donoghue <pure.logic at nexus-software.ie>

Thanks.

Johan


More information about the devel mailing list