On Tue, May 25, 2021 at 8:35 PM Christoph Hellwig [off-list ref] wrote:

On Tue, May 25, 2021 at 12:24:07PM +0000, guoren@kernel.org wrote:

quoted

From: Guo Ren <redacted>

Use static_branch_unlikely(&use_asid_allocator) to keep the origin
tlb flush style, so it's no effect on the existing machine. Here
are the optimized functions:
 - flush_tlb_mm
 - flush_tlb_page
 - flush_tlb_range

All above are based on the below new implement functions:
 - __sbi_tlb_flush_range_asid
 - local_flush_tlb_range_asid

This mentiones what functions you're changing, but not what the
substantial change is, and more importantly why you change it.

quoted

+static inline void local_flush_tlb_range_asid(unsigned long start, unsigned long size,
+                                           unsigned long asid)

Crazy long line.  Should be:

static inline void local_flush_tlb_range_asid(unsigned long start,
                unsigned long size, unsigned long asid)

quoted

+{
+     unsigned long tmp = start & PAGE_MASK;
+     unsigned long end = ALIGN(start + size, PAGE_SIZE);
+
+     if (size == -1) {
+             __asm__ __volatile__ ("sfence.vma x0, %0" : : "r" (asid) : "memory");
+             return;

Please split the global (size == -1) case into separate helpers.

Do you mean:
        if (size == -1) {
                __asm__ __volatile__ ("sfence.vma x0, %0"
                                :
                                : "r" (asid)
                                : "memory");
        } else {
                for (tmp = start & PAGE_MASK; tmp < end; tmp += PAGE_SIZE) {
                        __asm__ __volatile__ ("sfence.vma %0, %1"
                                        :
                                        : "r" (tmp), "r" (asid)
                                        : "memory");
                        tmp += PAGE_SIZE;
                }
        }

quoted

+     while(tmp < end) {

Missing whitespace befre the (.

Also I think this would read nicer as:

        for (tmp = start & PAGE_MASK; tmp < end; tmp += PAGE_SIZE)

quoted

+static void __sbi_tlb_flush_range_asid(struct cpumask *cmask, unsigned long start,
+                                    unsigned long size, unsigned long asid)

Another overly long line.

Also for all thee __sbi_* functions, why the __ prefix?

I just follow the previous coding convention by __sbi_tlb_flush_range.
If you don't like it, I think it should be another coding convention
patchset.
This patchset is only to add the functions of tlb_flush_with_asid.

quoted

+     if (cpumask_any_but(cmask, cpuid) >= nr_cpu_ids) {
+             local_flush_tlb_range_asid(start, size, asid);
+     } else {
+             riscv_cpuid_to_hartid_mask(cmask, &hmask);
+             sbi_remote_sfence_vma_asid(cpumask_bits(&hmask), start, size, asid);

Another long line (and a few more later).

-- 
Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/