From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrea Parri Date: Sat, 16 Mar 2024 12:36:23 +0100 Subject: [PATCH 1/5] riscv: Add Zawrs support for spinlocks In-Reply-To: <20240315134009.580167-8-ajones@ventanamicro.com> References: <20240315134009.580167-7-ajones@ventanamicro.com> <20240315134009.580167-8-ajones@ventanamicro.com> Message-ID: List-Id: To: kvm-riscv@lists.infradead.org MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit > +config RISCV_ISA_ZAWRS > + bool "Zawrs extension support for more efficient busy waiting" > + depends on RISCV_ALTERNATIVE > + default y > + help > + Enable the use of the Zawrs (wait for reservation set) extension > + when available. > + > + The Zawrs extension instructions (wrs.nto and wrs.sto) are used for > + more efficient busy waiting. Maybe mention that this is about _power_-efficiency? In the discussion of the previous iteration, I suggested [1]: The Zawrs extension defines a pair of instructions to be used in polling loops that allows a core to enter a low-power state and wait on a store to a memory location. (from the Zawrs spec -- I remain open to review other suggestiongs). > +#define ZAWRS_WRS_NTO ".long 0x00d00073" > +#define ZAWRS_WRS_STO ".long 0x01d00073" In the discussion of the previous iteration, you observed [2]: I'd prefer we use insn-def.h to define instructions, rather than scatter .long's around, but since this instruction doesn't have any inputs, then I guess it's not so important to use insn-def.h. So that "preference" doesn't apply to the instructions at stake? Or is not "important"? No real objections to barrier.h, trying to understand the rationale. > +#define ALT_WRS_NTO() \ > + __asm__ __volatile__ (ALTERNATIVE( \ > + "nop\n", ZAWRS_WRS_NTO "\n", \ > + 0, RISCV_ISA_EXT_ZAWRS, CONFIG_RISCV_ISA_ZAWRS)) > +#define ALT_WRS_STO() \ > + __asm__ __volatile__ (ALTERNATIVE( \ > + "nop\n", ZAWRS_WRS_STO "\n", \ > + 0, RISCV_ISA_EXT_ZAWRS, CONFIG_RISCV_ISA_ZAWRS)) > + > #define RISCV_FENCE(p, s) \ > __asm__ __volatile__ ("fence " #p "," #s : : : "memory") FYI, this hunk/patch conflicts with Eric's changes [3]. > +#define ___smp_load_reservedN(attr, ptr) \ > +({ \ > + typeof(*ptr) ___p1; \ > + \ > + __asm__ __volatile__ ("lr." attr " %[p], %[c]\n" \ > + : [p]"=&r" (___p1), [c]"+A"(*ptr)); \ > + ___p1; \ > +}) > + > +#define __smp_load_reserved_relaxed(ptr) \ > +({ \ > + typeof(*ptr) ___p1; \ > + \ > + if (sizeof(*ptr) == sizeof(int)) \ > + ___p1 = ___smp_load_reservedN("w", ptr); \ > + else if (sizeof(*ptr) == sizeof(long)) \ > + ___p1 = ___smp_load_reservedN("d", ptr); \ > + else \ > + compiletime_assert(0, \ > + "Need type compatible with LR/SC instructions for " \ > + __stringify(ptr)); \ > + ___p1; \ > +}) In the discussion of the previous iteration, you observed [2]: It's more common to use a switch for these things, [...] something like the macros in arch/riscv/include/asm/cmpxchg.h. Can we stick with that pattern? Along the same lines (#codingstyle), notice that ___smp_load_reservedN() would become one of the first uses of the asmSymbolicName syntax in the arch/riscv/ directory. So again, why not stick to the common style? something's wrong with it? > +#define smp_cond_load_relaxed(ptr, cond_expr) \ > +({ \ > + typeof(ptr) __PTR = (ptr); \ > + __unqual_scalar_typeof(*ptr) VAL; \ > + \ > + VAL = READ_ONCE(*__PTR); \ > + if (!cond_expr) { \ > + for (;;) { \ > + VAL = __smp_load_reserved_relaxed(__PTR); \ > + if (cond_expr) \ > + break; \ > + ALT_WRS_STO(); \ > + } \ > + } \ > + (typeof(*ptr))VAL; \ > +}) > + > +#define smp_cond_load_acquire(ptr, cond_expr) \ > +({ \ > + typeof(ptr) __PTR = (ptr); \ > + __unqual_scalar_typeof(*ptr) VAL; \ > + \ > + VAL = smp_load_acquire(__PTR); \ > + if (!cond_expr) { \ > + for (;;) { \ > + VAL = __smp_load_reserved_acquire(__PTR); \ > + if (cond_expr) \ > + break; \ > + ALT_WRS_STO(); \ > + } \ > + } \ > + (typeof(*ptr))VAL; \ > +}) In the discussion of the previous iteration, you observed [2]: I guess this peeling off of the first iteration is because it's expected that the load generated by READ_ONCE() is more efficient than lr.w/d? If we're worried about unnecessary use of lr.w/d, then shouldn't we look for a solution that doesn't issue those instructions when we don't have the Zawrs extension? To which Palmer replied (apparently, agreeing with your remarks) [4]: I haven't looked at the patch, but I'd expect we NOP out the whole LR/WRS sequence? I don't remember any reason to have the load reservation without the WRS, [...] Unfortunately, this submission makes no mention to those comments and, more importantly, to the considerations/tradeoffs which have led you to submit different changes. In submitting-patches.rst's words, Review comments or questions that do not lead to a code change should almost certainly bring about a comment or changelog entry so that the next reviewer better understands what is going on. Andrea P.S. BTW, not too far from the previous recommendation/paragraph is: When sending a next version, [...] Notify people that commented on your patch about new versions by adding them to the patches CC list. [1] https://lore.kernel.org/lkml/ZTE7eUyrb8+J+ORB at andrea [2] https://lore.kernel.org/lkml/20230524-35efcabbbfd6a1ef83865fb4 at orel [3] https://lore.kernel.org/lkml/20240217131206.3667544-1-ericchancf at google.com [4] https://lore.kernel.org/lkml/mhng-d92f84d8-03db-4fb1-93c3-0d5bfbe7a796 at palmer-ri-x1c9a