linux-kernel - Re: [PATCH] softirq: Remove raise_softirq from tasklet_action

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <28cbffc9-dcd4-60ac-897a-d1b1604de22c@quicinc.com>
Date:   Mon, 21 Feb 2022 16:48:19 +0530
From:   Mukesh Ojha <quic_mojha@...cinc.com>
To:     Frederic Weisbecker <frederic@...nel.org>
CC:     <linux-kernel@...r.kernel.org>, <peterz@...radead.org>,
        <tglx@...utronix.de>, <paulmck@...nel.org>, <will@...nel.org>,
        <dave@...olabs.net>
Subject: Re: [PATCH] softirq: Remove raise_softirq from
 tasklet_action_common()


On 2/9/2022 4:34 AM, Frederic Weisbecker wrote:
> On Sat, Feb 05, 2022 at 06:43:25PM +0530, Mukesh Ojha wrote:
>> Think about a scenario when all other cores are in suspend
>> and one core is only running ksoftirqd and it is because
>> some client has invoked tasklet_hi_schedule() only once
>> during that phase.
>>
>> tasklet_action_common() handles that softirq and marks the
>> same softirq as pending again. And due to that core keeps
>> running the softirq handler [1] forever and it is not able to
>> go to suspend.
>>
>> We can get rid of raising softirq from tasklet handler.
>>
>> [1]
>> <idle>-0    [003]   13058.769081:  softirq_entry   vec=0  action=HI_SOFTIRQ
>> <idle>-0     [003]  13058.769085: softirq_raise:        vec=0 [action=HI_SOFTIRQ]
>> <idle>-0    [003]   13058.769087:  softirq_exit   vec=0  action=HI_SOFTIRQ
>> <idle>-0    [003]   13058.769091:  softirq_entry   vec=0  action=HI_SOFTIRQ
>> <idle>-0     [003]  13058.769094: softirq_raise:        vec=0 [action=HI_SOFTIRQ]
>> <idle>-0    [003]   13058.769097:  softirq_exit   vec=0  action=HI_SOFTIRQ
>> <idle>-0    [003]   13058.769100:  softirq_entry   vec=0  action=HI_SOFTIRQ
>> <idle>-0     [003]  13058.769103: softirq_raise:        vec=0 [action=HI_SOFTIRQ]
>> <idle>-0    [003]   13058.769106:  softirq_exit   vec=0  action=HI_SOFTIRQ
>> <idle>-0    [003]   13058.769109:  softirq_entry   vec=0  action=HI_SOFTIRQ
>> <idle>-0    [003]   13059.058923:  softirq_entry   vec=0  action=HI_SOFTIRQ
>> ...
>> ..
>> ..
>> ..
>>
>> <idle>-0    [003]   13059.058951:  softirq_entry   vec=0  action=HI_SOFTIRQ
>> <idle>-0     [003]  13059.058954: softirq_raise:        vec=0 [action=HI_SOFTIRQ]
>> <idle>-0    [003]   13059.058957:  softirq_exit   vec=0  action=HI_SOFTIRQ
>> <idle>-0    [003]   13059.058960:  softirq_entry   vec=0  action=HI_SOFTIRQ
>> <idle>-0     [003]  13059.058963: softirq_raise:        vec=0 [action=HI_SOFTIRQ]
>> <idle>-0    [003]   13059.058966:  softirq_exit   vec=0  action=HI_SOFTIRQ
>> <idle>-0    [003]   13059.058969:  softirq_entry   vec=0  action=HI_SOFTIRQ
>> <idle>-0     [003]  13059.058972: softirq_raise:        vec=0 [action=HI_SOFTIRQ]
>> <idle>-0    [003]   13059.058975:  softirq_exit   vec=0  action=HI_SOFTIRQ
>> <idle>-0    [003]   13059.058978:  softirq_entry   vec=0  action=HI_SOFTIRQ
>> <idle>-0     [003]  13059.058981: softirq_raise:        vec=0 [action=HI_SOFTIRQ]
>> <idle>-0    [003]   13059.058984:  softirq_exit   vec=0  action=HI_SOFTIRQ
>> <idle>-0    [003]   13059.058987:  softirq_entry   vec=0  action=HI_SOFTIRQ
>> <idle>-0     [003]  13059.058990: softirq_raise:        vec=0 [action=HI_SOFTIRQ]
>> <idle>-0    [003]   13059.058993:  softirq_exit   vec=0  action=HI_SOFTIRQ
>> <idle>-0    [003]   13059.058996:  softirq_entry   vec=0  action=HI_SOFTIRQ
>> <idle>-0     [003]  13059.059000: softirq_raise:        vec=0 [action=HI_SOFTIRQ]
>> <idle>-0    [003]   13059.059002:  softirq_exit   vec=0  action=HI_SOFTIRQ
>>
>> Signed-off-by: Mukesh Ojha <quic_mojha@...cinc.com>
>> ---
>>   kernel/softirq.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/kernel/softirq.c b/kernel/softirq.c
>> index 41f4709..d3e6fb9 100644
>> --- a/kernel/softirq.c
>> +++ b/kernel/softirq.c
>> @@ -795,7 +795,6 @@ static void tasklet_action_common(struct softirq_action *a,
>>   		t->next = NULL;
>>   		*tl_head->tail = t;
>>   		tl_head->tail = &t->next;
>> -		__raise_softirq_irqoff(softirq_nr);
>>   		local_irq_enable();
> That requeue happens when the tasklet is already executing on some other CPU
> or when it has been disabled through tasklet_disable().
>
> So you can't just remove that line or you'll break everything.
>
> It would be nice to identify which tasklet keeps being requeued. Is it because
> something called tasklet_disable() to it and never called back tasklet_enable() ?

Hi @Frederic,

Thanks for the reply.
Suppose a scenario where a tasklet is scheduled/queued from one client 
and before running of tasklet handler, same tasklet gets
disabled from some other cpu.
In this scenario, while the handlers runs it will be keep on marking the 
softirq pending even though tasklet is disabled.
Tasklet will be enabled but after coming out of low power mode.
Will it look to be valid case ?

-Mukesh


>
> Thanks.