[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <871qdh32vp.fsf@cloudflare.com>
Date: Thu, 26 Oct 2023 15:05:26 +0200
From: Jakub Sitnicki <jakub@...udflare.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: linux-kernel@...r.kernel.org, kernel-team@...udflare.com,
Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
Subject: Re: [PATCH] genirq: Own affinity hint
On Wed, Oct 25, 2023 at 10:10 PM +02, Thomas Gleixner wrote:
> On Wed, Oct 25 2023 at 16:15, Jakub Sitnicki wrote:
>> @@ -55,26 +55,33 @@ static int alloc_masks(struct irq_desc *desc, int node)
>> {
>> if (!zalloc_cpumask_var_node(&desc->irq_common_data.affinity,
>> GFP_KERNEL, node))
>> - return -ENOMEM;
>> + goto err_affinity;
>> + if (!zalloc_cpumask_var_node(&desc->irq_common_data.affinity_hint,
>> + GFP_KERNEL, node))
>
> This gets allocated for every interrupt descriptor but only a few or
> even none will ever use it. Seriously no.
Makes sense. I wanted to start the simplest possible approach first.
I expect allocating it lazily will be more involved - have to cover the
not-allocated case.
But thinking about it some more - perhaps what makes more sense is, for
irq_set_affinity[_and]_hint users who don't want to bother with managing
a cpumask on their side, to switch to irq_set_affinity.
That interface doesn't require the cpumask to outlive the call, AFAICT.
After all, setting the affinity hint only buys you an ability to read it
out from /proc/irq/<N>/affinity_hint. Same information is available from
/proc/irq/<N>/smp_affinity. Please set me straight, if I'm missing
something here.
>
>> + goto err_affinity_hint;
>>
>> #ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
>> if (!zalloc_cpumask_var_node(&desc->irq_common_data.effective_affinity,
>> - GFP_KERNEL, node)) {
>> - free_cpumask_var(desc->irq_common_data.affinity);
>> - return -ENOMEM;
>> - }
>> + GFP_KERNEL, node))
>> + goto err_effective_affinity;
>> #endif
>>
>> #ifdef CONFIG_GENERIC_PENDING_IRQ
>> - if (!zalloc_cpumask_var_node(&desc->pending_mask, GFP_KERNEL, node)) {
>> -#ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
>> - free_cpumask_var(desc->irq_common_data.effective_affinity);
>> -#endif
>> - free_cpumask_var(desc->irq_common_data.affinity);
>> - return -ENOMEM;
>> - }
>> + if (!zalloc_cpumask_var_node(&desc->pending_mask, GFP_KERNEL, node))
>> + goto err_pending_mask;
>> #endif
>> return 0;
>> +
>> +err_pending_mask:
>
> How is this supposed to compile with CONFIG_GENERIC_PENDING_IRQ=n ?
>
>> +#ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
>> + free_cpumask_var(desc->irq_common_data.effective_affinity);
>> +#endif
>> +err_effective_affinity:
>
> and this with CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK=n ?
If it wasn't for the unused label (my bad) both cases LGTM.
But I double checked. If I force feed it through the preprocessor:
* CONFIG_GENERIC_PENDING_IRQ=n :
static int alloc_masks(struct irq_desc *desc, int node)
{
if (!zalloc_cpumask_var_node(&desc->irq_common_data.affinity,
((( gfp_t)(0x400u|0x800u)) | (( gfp_t)0x40u) | (( gfp_t)0x80u)), node))
goto err_affinity;
if (!zalloc_cpumask_var_node(&desc->irq_common_data.affinity_hint,
((( gfp_t)(0x400u|0x800u)) | (( gfp_t)0x40u) | (( gfp_t)0x80u)), node))
goto err_affinity_hint;
if (!zalloc_cpumask_var_node(&desc->irq_common_data.effective_affinity,
((( gfp_t)(0x400u|0x800u)) | (( gfp_t)0x40u) | (( gfp_t)0x80u)), node))
goto err_effective_affinity;
return 0;
err_pending_mask:
free_cpumask_var(desc->irq_common_data.effective_affinity);
err_effective_affinity:
free_cpumask_var(desc->irq_common_data.affinity_hint);
err_affinity_hint:
free_cpumask_var(desc->irq_common_data.affinity);
err_affinity:
return -12;
}
* CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK=n :
static int alloc_masks(struct irq_desc *desc, int node)
{
if (!zalloc_cpumask_var_node(&desc->irq_common_data.affinity,
((( gfp_t)(0x400u|0x800u)) | (( gfp_t)0x40u) | (( gfp_t)0x80u)), node))
goto err_affinity;
if (!zalloc_cpumask_var_node(&desc->irq_common_data.affinity_hint,
((( gfp_t)(0x400u|0x800u)) | (( gfp_t)0x40u) | (( gfp_t)0x80u)), node))
goto err_affinity_hint;
if (!zalloc_cpumask_var_node(&desc->pending_mask,
((( gfp_t)(0x400u|0x800u)) | (( gfp_t)0x40u) | (( gfp_t)0x80u)), node))
goto err_pending_mask;
return 0;
err_pending_mask:
err_effective_affinity:
free_cpumask_var(desc->irq_common_data.affinity_hint);
err_affinity_hint:
free_cpumask_var(desc->irq_common_data.affinity);
err_affinity:
return -12;
}
I did forget, however, to clean up the "old" affinity_hint field from
irq_desc struct.
Anyway - not planning on sending a v2, unless you see value in doing a
lazy allocation of the hint from within irq subsys.
Thanks for feedback.
Powered by blists - more mailing lists