[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <bedb35be-100b-95fb-64e4-b38edd3cd327@huawei.com>
Date: Wed, 6 Aug 2025 14:39:35 +0800
From: Jinjie Ruan <ruanjinjie@...wei.com>
To: Ada Couprie Diaz <ada.coupriediaz@....com>
CC: <catalin.marinas@....com>, <will@...nel.org>, <oleg@...hat.com>,
<sstabellini@...nel.org>, <mark.rutland@....com>, <puranjay@...nel.org>,
<broonie@...nel.org>, <mbenes@...e.cz>, <ryan.roberts@....com>,
<akpm@...ux-foundation.org>, <chenl311@...natelecom.cn>,
<anshuman.khandual@....com>, <kristina.martsenko@....com>,
<liaochang1@...wei.com>, <ardb@...nel.org>, <leitao@...ian.org>,
<linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
<xen-devel@...ts.xenproject.org>
Subject: Re: [PATCH -next v7 5/7] arm64: entry: Refactor
preempt_schedule_irq() check code
On 2025/8/5 23:06, Ada Couprie Diaz wrote:
> Hi Jinjie,
>
> On 29/07/2025 02:54, Jinjie Ruan wrote:
>> ARM64 requires an additional check whether to reschedule on return
>> from interrupt. So add arch_irqentry_exit_need_resched() as the default
>> NOP implementation and hook it up into the need_resched() condition in
>> raw_irqentry_exit_cond_resched(). This allows ARM64 to implement
>> the architecture specific version for switching over to
>> the generic entry code.
> I was a bit confused by this, as I didn't see the link with the generic
> entry
> given you implement `raw_irqentry_exit_cond_resched()` in arch/arm64
> as well in this patch : I expected the arm64 implementation to be added.
> I share more thoughts below.
>
> What do you think about something along those lines ?
>
> Compared to the generic entry code, arm64 does additional checks
> when deciding to reschedule on return from an interrupt.
> Introduce arch_irqentry_exit_need_resched() in the need_resched()
> condition
> of the generic raw_irqentry_exit_cond_resched(), with a NOP default.
> This will allow arm64 to implement its architecture specific checks
> when
> switching over to the generic entry code.
>
>> [...]
>> diff --git a/kernel/entry/common.c b/kernel/entry/common.c
>> index b82032777310..4aa9656fa1b4 100644
>> --- a/kernel/entry/common.c
>> +++ b/kernel/entry/common.c
>> @@ -142,6 +142,20 @@ noinstr irqentry_state_t irqentry_enter(struct
>> pt_regs *regs)
>> return ret;
>> }
>> +/**
>> + * arch_irqentry_exit_need_resched - Architecture specific need
>> resched function
>> + *
>> + * Invoked from raw_irqentry_exit_cond_resched() to check if need
>> resched.
> Very nit : "to check if resched is needed." ?
>> + * Defaults return true.
>> + *
>> + * The main purpose is to permit arch to skip preempt a task from an
>> IRQ.
> If feel that "to avoid preemption of a task" instead of "to skip preempt
> a task"
> would make this much clearer, what do you think ?
>> + */
>> +static inline bool arch_irqentry_exit_need_resched(void);
>> +
>> +#ifndef arch_irqentry_exit_need_resched
>> +static inline bool arch_irqentry_exit_need_resched(void) { return
>> true; }
>> +#endif
>> +
>
> I've had some trouble reviewing this patch : on the one hand because
> I didn't notice `arch_irqentry_exit_need_resched()` was added in
> the common entry code, which is on me !
> On the other hand, I felt that the patch itself was a bit disconnected :
> we add `arch_irqentry_exit_need_resched()` in the common entry code,
> with a default NOP, but in the same function we add to arm64,
> while mentioning that this is for arm64's additional checks,
> which we only implement in patch 7.
Yes, it does.
>
> Would it make sense to move the `arch_irqentry_exit_need_resched()`
> part of the patch to patch 7, so that the introduction and
> arch-specific implementation appear together ?
> To me it seems easier to wrap my head around, as it would look like
> "Move arm64 to generic entry, but it does additional checks : add a new
> arch-specific function controlling re-scheduling, defaulting to true,
> and implement it for arm64". I feel it could help making patch 7's
> commit message clearer as well.
>
> From what I gathered on the archive `arch_irqentry_exit_need_resched()`
> being added here was suggested previously, so others might not have the
> same opinion.
Yes, introduce `arch_irqentry_exit_need_resched()` here may help
understand the patch's refactoring purpose.
>
> Maybe improving the commit message and comment for this would be enough
> as well, as per my suggestions above.
Thank you! I'll improve the commit message and comment.
>
>
> Otherwise the changes make sense and I don't see any functional issues !
>
> Thanks,
> Ada
>
>
Powered by blists - more mailing lists