linux-kernel - Re: [RFC PATCH v3] genirq/affinity: Create and transfer more irq desc info by a new structure

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]

Message-ID: <0aaa0eeb-2c8e-71e2-bf92-87103735b2b6@gmail.com>
Date:   Thu, 29 Nov 2018 22:49:54 +0800
From:   Dou Liyang <douliyangs@...il.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>, kashyap.desai@...adcom.com,
        shivasharan.srikanteshwara@...adcom.com, sumit.saxena@...adcom.com,
        ming.lei@...hat.com, Christoph Hellwig <hch@....de>,
        bhelgaas@...gle.com, linux-pci@...r.kernel.org
Subject: Re: [RFC PATCH v3] genirq/affinity: Create and transfer more irq desc
 info by a new structure

Hi Thomas,

On 2018/11/29 6:03, Thomas Gleixner wrote:

>> +		affi_desc = kcalloc(nvec, sizeof(*affi_desc), GFP_KERNEL);
> 
> Why do you want to do that separate allocation here? Just let

I thought the irq_create_affinity_desc() also can be called by other 
functions which may convert cpumasks to irq_affinity_desc, such as
__devm_irq_alloc_descs().

Now, I know I was wrong, will modify it.

> irq_create_affinity_masks() allocate an array of affinity descriptors and
> use that. There is no point in copying that stuff over and over. Setting
> the flag field can be done in the existing function as well.

> Can you please change the function signature and fixup the callers, if
> there are any of them? Copying this over and over is horrible.

I have searched, no one calls __devm_irq_alloc_descs, it may be called
by some users' own modules or drives.

yes, I will change it.

> struct irq_affinity_desc {
> 	struct cpumask	masks;
> 	unsigned int	managed : 1; > };

yes, BTW, If the following is more fit for irq_affinity_desc:

s/masks/mask/
s/managed/is_managed/

> 

> You can spare a lot of pointless churn by just keeping the 'affinity' name
> and only changing the struct type. The compiler will catch all places which
> need to be fixed and 'affinity' is generic enough to be used with the new
> struct type as well. As Bjorn said, even 'masks' is fine.

Yes, I see

Thanks,
	dou