Re: [RFC patch 14/19] bpf: Use migrate_disable() in hashtab code

On Wed, Feb 19, 2020 at 01:49:57AM +0100, Thomas Gleixner wrote:

Alexei,

Alexei Starovoitov [off-list ref] writes:

quoted

Overall looks great.
Thank you for taking time to write commit logs and detailed cover letter.
I think s/__this_cpu_inc/this_cpu_inc/ is the only bit that needs to be
addressed for it to be merged.
There were few other suggestions from Mathieu and Jakub.
Could you address them and resend?

I have them fixed up already, but I was waiting for further
comments. I'll send it out tomorrow morning as I'm dead tired by now.

quoted

I saw patch 1 landing in tip tree, but it needs to be in bpf-next as well
along with the rest of the series. Does it really need to be in the tip?
I would prefer to take the whole thing and avoid conflicts around
migrate_disable() especially if nothing in tip is going to use it in this
development cycle. So just drop patch 1 from the tip?

I'll add patch 2 to a tip branch as well and I'll give you a tag to pull
into BPF (which has only those two commits). 

That works too.

quoted

Regarding
union {
   raw_spinlock_t  raw_lock;
   spinlock_t      lock;
};
yeah. it's not pretty, but I also don't have better ideas.

Yeah. I really tried hard to avoid it, but the alternative solution was
code duplication which was even more horrible.

quoted

Regarding migrate_disable()... can you enable it without the rest of RT?
I haven't seen its implementation. I suspect it's scheduler only change?
If I can use migrate_disable() without RT it will help my work on sleepable
BPF programs. I would only have to worry about rcu_read_lock() since
preempt_disable() is nicely addressed.

You have to talk to Peter Zijlstra about this as this is really
scheduler relevant stuff. FYI, he undamentaly hates migrate_disable()
from a schedulabilty POV, but as with the above lock construct the
amount of better solutions is also close to zero.

I would imagine that migrate_disable is like temporary cpu pinning. The
scheduler has to have all the logic to make scheduling decisions quickly in
presence of pinned tasks and additional migrate_disable shouldn't introduce
slowdowns or complexity to critical path. Anyway, we'll discuss further
when migrate_disable patches hit mailing lists.