Received-SPF: pass (google.com: domain of linux-kernel+bounces-72048-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: Thomas Gleixner <tglx@linutronix.de>
To: Bitao Hu <yaoma@linux.alibaba.com>
Cc: linux-kernel@vger.kernel.org, Doug Anderson <dianders@chromium.org>,
 akpm@linux-foundation.org, Petr Mladek <pmladek@suse.com>,
 kernelfans@gmail.com, Liu Song <liusong@linux.alibaba.com>,
 yaoma@linux.alibaba.com
Subject: Re: [PATCHv7 2/2] watchdog/softlockup: report the most frequent
 interrupts
In-Reply-To: <a7dd72c5-ed0a-4270-b3a7-5775037703e4@linux.alibaba.com>
References: <87mss2dkxo.ffs@tglx>
 <a7dd72c5-ed0a-4270-b3a7-5775037703e4@linux.alibaba.com>
Date: Mon, 19 Feb 2024 23:15:13 +0100
Message-ID: <87wmr0hzim.ffs@tglx>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

On Mon, Feb 19 2024 at 17:12, Bitao Hu wrote:
> On 2024/2/15 19:30, Thomas Gleixner wrote:
>> On Wed, Feb 14 2024 at 10:14, Bitao Hu wrote:
>>> +		for (i = 0; i < NUM_HARDIRQ_REPORT; i++) {
>>> +			if (irq_counts_sorted[i].irq == -1)
>>> +				break;
>>> +
>>> +			desc = irq_to_desc(irq_counts_sorted[i].irq);
>>> +			if (desc && desc->action)
>>> +				printk(KERN_CRIT "\t#%u: %-10u\tirq#%d(%s)\n",
>>> +				       i + 1, irq_counts_sorted[i].counts,
>>> +				       irq_counts_sorted[i].irq, desc->action->name);
>> 
>> You cannot dereference desc->action here:
>> 
>>      1) It can be NULL'ed between check and dereference.
>> 
>>      2) Both 'action' and 'action->name' can be freed in parallel
>> 
>> And no, you cannot take desc->lock here to prevent this. Stop fiddling
>> in the internals of interrupt descriptors.
>
> Thanks for your analysis. However, I have a question. 'action->name'
> cannot be accessed here, and it seems that merely outputting the
> irq number provides insufficient information?

That's what you can access without risk. It's better than nothing, no?

>> and the analysis boils down to:
>> 
>>          u64 cnt, sorted[3] = {};
>>          unsigned int irq, i;
>> 
>>      	for_each_active_irq(irq) {
>>          	cnt = kstat_get_irq_since_snapshot(irq);
>> 
>> 		if (cnt) {
>>                       	for (cnt = (cnt << 32) + irq, i = 0; i < 3; i++) {
>>                  		if (cnt > sorted[i])
>>                          		swap(cnt, sorted[i]);
> Hmm, I think the approach here isn't optimal. If some interrupts
> have the same count, then it effectively results in sorting by the
> irq number. Is my understanding correct?

Sure, but what's the problem? If two interrupts have the same count then
the ordering is pretty much irrelevant, no?

Thanks,

        tglx