linux-kernel - Re: [PATCH] genirq: Own affinity hint

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

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.