Hi Charles,
On Fri, Apr 22, 2022 at 11:23:01PM +0100, Charles-François Natali wrote:
> > Regarding your patch, is there a way to make that a bit more succinct,
> > without introducing all of those helper functions? It seems awfully
> > verbose for something that seems like a matter of replacing the online
> > mask with the housekeeping mask.
>
> Indeed, I wasn't really happy about that.
> The reason I've written those helper functions is that the housekeeping mask
> includes possible CPUs (cpu_possible_mask), so unfortunately it's not just a
> matter of e.g. replacing cpu_online_mask with
> housekeeping_cpumask(HK_FLAG_DOMAIN), we have to perform an AND
> whenever we compute the weight, find the next CPU in the mask etc.
>
> And I'd rather have the operations and mask in a single location instead of
> scattered throughout the code, to make it easier to understand and maintain.
>
> Happy to change to something more inline though, or open to suggestions.
Probably more inlined, yea. A simpler version of your patch would
probably be something like this, right?
diff --git a/drivers/net/wireguard/queueing.h b/drivers/net/wireguard/queueing.h
index 583adb37ee1e..b3117cdd647d 100644
--- a/drivers/net/wireguard/queueing.h
+++ b/drivers/net/wireguard/queueing.h
@@ -112,6 +112,8 @@ static inline int wg_cpumask_choose_online(int *stored_cpu, unsigned int id)
cpu = cpumask_first(cpu_online_mask);
for (i = 0; i < cpu_index; ++i)
cpu = cpumask_next(cpu, cpu_online_mask);
+ while (!housekeeping_test_cpu(cpu, HK_???))
+ cpu = cpumask_next(cpu, cpu_online_mask);
*stored_cpu = cpu;
}
return cpu;
@@ -128,7 +130,7 @@ static inline int wg_cpumask_next_online(int *next)
{
int cpu = *next;
- while (unlikely(!cpumask_test_cpu(cpu, cpu_online_mask)))
+ while (unlikely(!cpumask_test_cpu(cpu, cpu_online_mask) && !housekeeping_test_cpu(cpu, HK_???)))
cpu = cpumask_next(cpu, cpu_online_mask) % nr_cpumask_bits;
*next = cpumask_next(cpu, cpu_online_mask) % nr_cpumask_bits;
return cpu;
However, from looking at kernel/sched/isolation.c a bit, I noticed that
indeed you're right that most of these functions (save one) are based on
cpu_possible_mask rather than cpu_online_mask. This is frustrating
because the code makes smart use of static branches to remain quick, but
ANDing housekeeping_cpumask() with cpu_online_mask would, in the fast
path, wind up ANDing cpu_online_mask with cpu_possible_mask, which is
silly and pointless. That makes me suspect that maybe the best approach
would be adding a relevant helper to kernel/sched/isolation.c, so that
the helper can then do the `if (static_branch_unlikely(&housekeeping_overridden))`
stuff internally.
Or maybe you'll do some measurements and decide that just [ab]using
housekeeping_test_cpu() like above is actually optimal? Not really sure
myself.
Anyway, I'll keep an eye out for your joint wireguard/padata series. Be
sure to CC the people who wrote the isolation & housekeeping code, as
they likely have opinions about this stuff (and certainly know more than
me about it).
Jason
