On Tue, 2022-11-29 at 13:10 +0100, Frederic Weisbecker wrote:

On Fri, Oct 14, 2022 at 01:27:25PM -0300, Leonardo Brás wrote:

quoted

Hello Frederic,

So, IIUC you are removing all flags composing nohz_full= parameter in favor of a
unified NOHZ_FULL flag. 

I am very new to the code, and I am probably missing the whole picture, but I
actually think it's a good approach to keep them split for a couple reasons:
1 - They are easier to understand in code (IMHO): 
"This cpu should not do this, because it's not able to do WQ housekeeping" looks
better than "because it's not in DOMAIN or NOHZ_FULL housekeeping"

A comment above each site may solve that.

Sure, but not having to leave comments would be better. Or am I missing
something?

quoted

2 - They are simpler for using: 
Suppose we have this function that should run at a WQ, but we want to keep them
out of the isolated cpus. If we have the unified flags, we need to combine both
DOMAIN and NOHZ_FULL bitmasks, and then combine it again with something like
cpu_online_mask. It usually means allocating a new cpumask_t, and also freeing
it afterwards.
If we have a single WQ flag, we can avoid the allocation altogether by using
for_each_cpu_and(), making the code much simpler.

I guess having a specific function for workqueues would arrange for it.

You mean keeping a WQ housekeeping bitmap? This could be a solution, but it
would affect only the WQ example.

quoted

3 - It makes easier to compose new isolation modes:
In case the future requires a new isolation mode that also uses the types of
isolation we currently have implemented, it would be much easier to just compose
it with the current HK flags, instead of having to go through all usages and do
a cpumask_and() there. Also, new isolation modes would make (2) worse.

Actually having a new feature merged in HK_NOHZ_FULL would make it easier to
handle as it avoids spreading cpumasks. I'm not sure I understand what you
mean.

IIUC, your queued patch merges the housekeeping types HK_TYPE_TIMER,
HK_TYPE_RCU, HK_TYPE_MISC, HK_TYPE_TICK, HK_TYPE_WQ and HK_TYPE_KTHREAD in a
single HK_TYPE_NOHZ_FULL.

Suppose in future we need a new isolation feature in cmdline, say 
isol_new=<cpulist>, and it works exactly like nohz_full=<cpulist>, but also
needs to isolate cpulist against something else, say doing X.

How would this get implemented? IIUC, following the same pattern:
- A new type HK_TYPE_ISOL_NEW would be created together with a cpumask,
- The new cpumask would be used to keep cpulist from doing X
- All places that use HK_TYPE_NOHZ_FULL bitmap for isolation would need to also
bitmask_and() the new cpumask. (sometimes needing a local cpumask_t)

Ok, there may be shortcuts for this, like keeping an intermediary bitmap, but
that can become tricky.

Other more complex example: New isolation feature isol_new2=<cpulist> behaves
like nohz_full=<cpulist>, keeps cpulist from doing X, but allows unbound RCU
work. Now it's even harder to have shortcuts from previous implementation.

What I am trying to defend here is that keeping the HK_type with the idea of
"things to get cpulist isolated from" works better for future implementations
than a single flag with a lot of responsibilities:
- A new type HK_TYPE_X would be created together with a cpumask,
- The new cpumask would be used to keep cpulist from doing X
- isol_new=<cpulist> is composed with the flags for what cpulist is getting
isolated.
- (No need to touch already implemented isolations.)

In fact, I propose that it works better for current implementations also:
The current patch (3/4) takes the WQ isolation responsibility from
HK_TYPE_DOMAIN and focus it in HK_TYPE_WQ, adding it to isolcpus=<cpulist>
flags. This avoids some cpumask_and()s, and a cpumask_t kzalloc, and makes the
code less complex to implement when we need to put isolation in further parts of
the code. (patch 4/4)

I am not sure if I am missing some important point here. 
Please let me know if it's the case. 

Thanks.

Thank you for replying!
Leo