linux-kernel - Re: [GIT PULL] target fixes for v3.13

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] [day] [month] [year] [list]

Date:	Sun, 19 Jan 2014 02:43:50 -0800
From:	"Nicholas A. Bellinger" <nab@...ux-iscsi.org>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Jens Axboe <axboe@...nel.dk>, Ingo Molnar <mingo@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	target-devel <target-devel@...r.kernel.org>,
	linux-scsi <linux-scsi@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Kent Overstreet <kmo@...erainc.com>
Subject: Re: [GIT PULL] target fixes for v3.13

On Fri, 2014-01-17 at 17:29 -0800, Linus Torvalds wrote:
> On Thu, Jan 16, 2014 at 4:09 PM, Nicholas A. Bellinger
> <nab@...ux-iscsi.org> wrote:
> >
> > This change allows the percpu_ida tag allocator to optionally use
> > interruptible sleep that iscsi-target expects, while still leaving the
> > functionality + interface for existing percpu_ida consumers unchanged.
> 
> I'm not pulling this. Passing in TASK_RUNNING to prepare_to_wait() is
> insane (because if it ever were to actually wait, that would be a
> bug), and afaik this would be the first time anybody ever does that.
> 
> Yes, yes, it may "work", but I'm not pulling that kind of hack just
> before a release.
> 
> Quite frankly, it looks like you want to have a tristate argument ("no
> wait", "wait interruptibly", "wait uninterruptibly") to that
> percpu_ida_alloc() function. Fine. But dammit, using this kind of
> hackery, and then having two *different* calling conventions (one
> mis-using the gfp_t for legacy reasons, and one now using the task
> state flags in odd ways) is just not acceptable.
> 
> Now, neither of those two is perfect, but I can see why you want to
> use the task state ones to say which kind of interruptible you want..
> But I really don't like suddenly having a
> prepare_to_wait(TASK_RUNNING) caller without any discussion, and I
> *really* don't like having two completely different models for this
> hack.
> 
> So quite frankly, I'd much prefer:
> 
>  - talk to the scheduler people, and make them aware of the fact that
> you are going to pass in TASK_RUNNING to prepare_to_wait(). It works
> with the code as-is, as long as you don't actually then wait.
> 
>  - Don't do that wrapper function with a totally different calling
> convention logic. Instead, just change all the callers explicitly.
> From a quick look, you really only have a couple of cases:
> 
>   (a) target/iscsi, which wants the new ternary argument
> 
>   (b) vhost/scsi.c, which uses GFP_ATOMIC and can be changed to TASK_RUNNABLE
> 
>   (c) block/blk-mq-tag.c, which already hates the current insane
> thing, and uses __GFP_WAIT and (gfp & ~__GFP_WAIT) and other hacks,
> and is obviously *very* aware of the internal hackery in the current
> percpu_ida_alloc() argument. So I'm getting the feeling that that
> whole thing might actually be *happier* with the TASK_xyz flags.
> 

Sure.

> 
> So I really think this needs cleanup, and that hacky "passing in
> TASK_RUNNING to prepare_to_wait()" needs to be made official. And yes,
> that implies that it's too late to try to push this through for 3.13,
> this goes into the next merge window and can be backported.
> 
> Added the appropriate people to the Cc..
> 

Just sent out a series as requested for review with CC's for Ingo +
Peter.

Jens, please review the blk-mq changes.

Thanks!

--nab

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