Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 24 Nov 2022 01:10:06 -0800
From:   Deepak Gupta <debug@rivosinc.com>
To:     Conor Dooley <conor.dooley@microchip.com>
Cc:     Guo Ren <guoren@kernel.org>, palmer@dabbelt.com,
        linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org,
        paul.walmsley@sifive.com, Jisheng Zhang <jszhang@kernel.org>
Subject: Re: [PATCH] riscv: VMAP_STACK overflow detection thread-safe
Message-ID: <20221124091006.GC1896875@debug.ba.rivosinc.com>
References: <20221124005006.1788315-1-debug@rivosinc.com>
 <CAJF2gTQ=Vr6neABtz9JSCei6oPEsyWTpb-Y=Rxt5jy6n1VEtGA@mail.gmail.com>
 <CAKC1njRfxNx8r5SP7qsbouCFfb1L4w63paQibb=cFCBhWDXNXA@mail.gmail.com>
 <CAJF2gTRC-qm76=aaLh75amO0JJLO2P4eZaJXH-TBSV0c7iqUBA@mail.gmail.com>
 <20221124071022.GA1149630@debug.ba.rivosinc.com>
 <Y38ydMoqmqOPfBs0@wendy>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <Y38ydMoqmqOPfBs0@wendy>
Precedence: bulk

On Thu, Nov 24, 2022 at 08:59:32AM +0000, Conor Dooley wrote:
>On Wed, Nov 23, 2022 at 11:10:22PM -0800, Deepak Gupta wrote:
>> On Thu, Nov 24, 2022 at 02:31:25PM +0800, Guo Ren wrote:
>> > On Thu, Nov 24, 2022 at 1:57 PM Deepak Gupta <debug@rivosinc.com> wrote:
>
>> > > On Wed, Nov 23, 2022 at 5:28 PM Guo Ren <guoren@kernel.org> wrote:
>
>> > > > On Thu, Nov 24, 2022 at 8:50 AM Deepak Gupta <debug@rivosinc.com> wrote:
>
>> > > >> Fixes: 31da94c25aea835ceac00575a9fd206c5a833fed
>> > > >
>> > > > The patch gives more significant change than the Fixes, and Fixes would expand to the previous stable versions. Please don't set it as a Fixes, but an improved OVERSTACK dead path performance feature.
>> > > >
>> > >
>> > > Not a performance feature but more like correctness.
>> > > If kernel died and two CPUs raced to kernel stack overflow,
>> > > death post-mortem should be straightforward.
>> > We already have had a fixup, and your patch likes a feature with a
>> > significant change.
>> > https://lore.kernel.org/linux-riscv/20221030124517.2370-1-jszhang@kernel.org/
>> > If it is for correctness, the simple lock is enough.
>>
>> Sure lock is enough. It's different way to solve the problem. But I don't
>> think it qualifies as significant change.
>
>Something to bear in mind is where in the cycle we are - there's likely
>just over a week left before v6.1.
>Since the lock is sufficient to fix the problem for v6.1, it's easy to
>view this patch as an optimisation or improvement that should go on top
>of that, smaller, patch.
>Especially when you have some questions yourself about the correctness
>for 32 bit!

Yes I'll have to revise the patch to accomodate 32bit.

>I've got no technical comment to make about the discussion here, but
>looking in from the "outside", that's the easy conclusion to jump to.
>
>
>>	REG_S x31, TASK_TI_SPILL_REG(tp)
>>	asm_per_cpu sp, overflow_stack, x31
>>	li x31, OVERFLOW_STACK_SIZE
>>	add sp, sp, x31
>>	REG_L x31, TASK_TI_SPILL_REG(tp)
>
>btw, for this sort of thing, could you please use some whitespace to
>align the operands? Makes things significantly more readable.
>

Noted. I'll do that.

>Thanks,
>Conor.
>