linux-kernel - Re: [PATCH] tty: Only wakeup the line discipline idle queue when queue is active

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130103183611.GA28225@redhat.com>
Date:	Thu, 3 Jan 2013 19:36:11 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Ivo Sieben <meltedpianoman@...il.com>
Cc:	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org,
	Andi Kleen <andi@...stfloor.org>,
	Preeti U Murthy <preeti@...ux.vnet.ibm.com>,
	Jiri Slaby <jslaby@...e.cz>,
	Alan Cox <alan@...rguk.ukuu.org.uk>,
	linux-serial@...r.kernel.org, Alan Cox <alan@...ux.intel.com>,
	Greg KH <gregkh@...uxfoundation.org>
Subject: Re: [PATCH] tty: Only wakeup the line discipline idle queue when
	queue is active

On 01/03, Ivo Sieben wrote:
>
> Oleg, Peter, Ingo, Andi & Preeti,
>
> 2013/1/2 Jiri Slaby <jslaby@...e.cz>:
> > On 01/02/2013 04:21 PM, Ivo Sieben wrote:
> >> I don't understand your responses: do you suggest to implement this
> >> "if active" behavior in:
> >> * A new wake_up function called wake_up_if_active() that is part of
> >> the waitqueue layer?
> >
> > Sounds good.
> >
> > --
> > js
> > suse labs
>
> I want to ask you 'scheduler' people for your opinion:
>
> Maybe you remember my previous patch where I suggested an extra
> 'waitqueue empty' check before entering the critical section of the
> wakeup() function (If you do not remember see
> https://lkml.org/lkml/2012/10/25/159)
>
> Finally Oleg responded that a lot of callers do
>
> 	if (waitqueue_active(q))
> 		wake_up(...);
>
> what made my patch pointless and adds a memory barrier.

Plus this change doesn't look 100% correct, at least in theory.

> I then decided
> to also implement the 'waitqueue_active' approach for my problem.

Well, if you ask me I think this is the best solution ;)

But I won't insist.

> But now I get a review comment by Jiri that he would like to hide this
> 'if active behavior' in a wake_up_if_active() kind of function. I
> think he is right that implementing this check in the wakeup function
> would clean things up, right?
>
> I would like to have your opinion on the following two suggestions:
> - We still can do the original patch on the wake_up() that I
> suggested. I then can do an additional code cleanup patch that removes
> the double 'waitqueue_active' call (a quick grep found about 150 of
> these waitqueue active calls) on several places in the code.

In this case we should also fix some users of add_wait_queue().

> - Or - as an alternative - I could add extra _if_active() versions of
> all wake_up() functions, that implement this extra test.

Not sure this will actually help to make the code cleaner. The last
patch you sent looks simple and clean. IMHO it doesn't make sense
to create _if_active helper for each wake_up*.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/