Re: [PATCH] sched: Revert 14a40ffccd61 ("sched: replacePF_THREAD_BOUND with PF_NO_SETAFFINITY")

[Ingo Molnar]

* Tejun Heo <htejun@xxxxxxxxx> wrote:

> Hello, guys.
> 
> On Mon, Dec 02, 2013 at 04:37:46PM +0100, Ingo Molnar wrote:
> > >  - We should strive to never consciously place artificial limitations on
> > >    kernel functionality; our main reason to place limitations should be
> > >    correctness.
> 
> It is a correctness issue tho.  We'd be promising, say, NUMA affinity
> and then possibly give out workers which aren't affine to the NUMA
> node. [...]

No, NUMA is a performance issue in this specific case, not a 
correctness issue. 'Correctness' for configuration details mostly 
means 'stuff does not crash'.

We try to give good, sensible, well performing defaults out of box, 
but otherwise we try to let root mis-configure (or improve!) their 
systems rather widely.

This is really fundamental: our affinity APIs are generic, and they 
should only ever be limited in such a drastic fashion in the very 
strict 'stuff crashes' case, and that is properly expressed via the 
PF_THREAD_BOUND flag.

(Your other arguments fail for similar reasons.)

> This is the same problem with bound workers.  Userland could be 
> actively breaking configured affinities of kworkers which may be 
> depended upon by its users and there's no way for userland to tell 
> which kworker is gonna serve which workqueue - there's no fixed 
> relationship between them, so it's not like userland can, say, 
> modify affinities on crypto kworkers in isolation.  Both userland 
> and kernel wouldn't have much idea of the impact of such fiddling.

That's only because 'private affinity' attributes detached scheduler 
affinity handling snuck into the workqueue code. It's bad design and 
it needs to be fixed - and the worst aspect is undone via this revert.

> > Peter's workqueue.c fixlet above to workqueue.c to this patch plus 
> > your Acked-by, so that the two changes are together?
> 
> The above would trigger the added warning if a new kworker is 
> created for a per-cpu workqueue while the cpu is down, which may 
> happen, so the patch itself is somewhat broken.

I'll wait for a new submission from Peter.

> I don't think this is the right path to accomodate HPC/RT use cases. 
> Let's please implement something proper if keeping unbound kworkers 
> off certain cpus is necessary.

We already have APIs to manage affinities, in the scheduler. 
Privatizing this function into workqueue.c is limiting and actively 
wrong.

Such APIs should be done properly, by extending existing scheduler 
APIs if needed, not by turning off the old APIs via a crude flag and 
creating some private, per subsystem implementation ...

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