- Linuxarm - mailweb.openeuler.org

Re: [RFC PATCH v3 6/9] hw/arm/virt-acpi-build: Use possible cpus in generation of MADT
by Salil Mehta 18 May '21

18 May '21

> From: Qemu-arm [mailto:qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org] > On Behalf Of Yanan Wang > Sent: Sunday, May 16, 2021 11:29 AM > To: Peter Maydell <peter.maydell(a)linaro.org>; Andrew Jones > <drjones(a)redhat.com>; Michael S . Tsirkin <mst(a)redhat.com>; Igor Mammedov > <imammedo(a)redhat.com>; Shannon Zhao <shannon.zhaosl(a)gmail.com>; Alistair > Francis <alistair.francis(a)wdc.com>; David Gibson > <david(a)gibson.dropbear.id.au>; qemu-devel(a)nongnu.org; qemu-arm(a)nongnu.org > Cc: Song Bao Hua (Barry Song) <song.bao.hua(a)hisilicon.com>; zhukeqian > <zhukeqian1(a)huawei.com>; yangyicong <yangyicong(a)huawei.com>; Zengtao (B) > <prime.zeng(a)hisilicon.com>; Wanghaibin (D) <wanghaibin.wang(a)huawei.com>; > yuzenghui <yuzenghui(a)huawei.com>; Paolo Bonzini <pbonzini(a)redhat.com>; > Philippe Mathieu-Daudé <philmd(a)redhat.com> > Subject: [RFC PATCH v3 6/9] hw/arm/virt-acpi-build: Use possible cpus in > generation of MADT > > When building ACPI tables regarding CPUs we should always build > them for the number of possible CPUs, not the number of present > CPUs. So we create gicc nodes in MADT for possible cpus and then > ensure only the present CPUs are marked ENABLED. Furthermore, it > also needed if we are going to support CPU hotplug in the future. Hi Yanan, Yes, these changes are part of the QEMU patch-set I floated last year. Link: https://www.mail-archive.com/qemu-devel@nongnu.org/msg712018.html Perhaps I am missing something, but how this patch is related to the vcpu topology support? Thanks > > Co-developed-by: Andrew Jones <drjones(a)redhat.com> > Signed-off-by: Andrew Jones <drjones(a)redhat.com> > Co-developed-by: Ying Fang <fangying1(a)huawei.com> > Signed-off-by: Ying Fang <fangying1(a)huawei.com> > Co-developed-by: Yanan Wang <wangyanan55(a)huawei.com> > Signed-off-by: Yanan Wang <wangyanan55(a)huawei.com> > --- > hw/arm/virt-acpi-build.c | 29 +++++++++++++++++++++++++---- > 1 file changed, 25 insertions(+), 4 deletions(-) > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > index a2d8e87616..4d64aeb865 100644 > --- a/hw/arm/virt-acpi-build.c > +++ b/hw/arm/virt-acpi-build.c > @@ -481,6 +481,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, > VirtMachineState *vms) > const int *irqmap = vms->irqmap; > AcpiMadtGenericDistributor *gicd; > AcpiMadtGenericMsiFrame *gic_msi; > + MachineClass *mc = MACHINE_GET_CLASS(vms); > + const CPUArchIdList *possible_cpus = > mc->possible_cpu_arch_ids(MACHINE(vms)); > + bool pmu; > int i; > > acpi_data_push(table_data, sizeof(AcpiMultipleApicTable)); > @@ -491,11 +494,21 @@ build_madt(GArray *table_data, BIOSLinker *linker, > VirtMachineState *vms) > gicd->base_address = cpu_to_le64(memmap[VIRT_GIC_DIST].base); > gicd->version = vms->gic_version; > > - for (i = 0; i < MACHINE(vms)->smp.cpus; i++) { > + for (i = 0; i < possible_cpus->len; i++) { > AcpiMadtGenericCpuInterface *gicc = acpi_data_push(table_data, > sizeof(*gicc)); > ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i)); > > + /* > + * PMU should have been either implemented for all CPUs or not, > + * so we only get information from the first CPU, which could > + * represent the others. > + */ > + if (i == 0) { > + pmu = arm_feature(&armcpu->env, ARM_FEATURE_PMU); > + } > + assert(!armcpu || arm_feature(&armcpu->env, ARM_FEATURE_PMU) == pmu); > + > gicc->type = ACPI_APIC_GENERIC_CPU_INTERFACE; > gicc->length = sizeof(*gicc); > if (vms->gic_version == 2) { > @@ -504,11 +517,19 @@ build_madt(GArray *table_data, BIOSLinker *linker, > VirtMachineState *vms) > gicc->gicv_base_address = > cpu_to_le64(memmap[VIRT_GIC_VCPU].base); > } > gicc->cpu_interface_number = cpu_to_le32(i); > - gicc->arm_mpidr = cpu_to_le64(armcpu->mp_affinity); > + gicc->arm_mpidr = cpu_to_le64(possible_cpus->cpus[i].arch_id); > gicc->uid = cpu_to_le32(i); > - gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED); > > - if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) { > + /* > + * ACPI spec says that LAPIC entry for non present CPU may be > + * omitted from MADT or it must be marked as disabled. Here we > + * choose to also keep the disabled ones in MADT. > + */ > + if (possible_cpus->cpus[i].cpu != NULL) { > + gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED); > + } > + > + if (pmu) { > gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ)); > } > if (vms->virt) { > -- > 2.19.1 >

1 0

Re: [RFC PATCH v3 4/4] hw/arm/virt: Parse -smp cluster parameter in virt_smp_parse
by Salil Mehta 17 May '21

17 May '21

> From: Qemu-devel > [mailto:qemu-devel-bounces+salil.mehta=huawei.com@nongnu.org] On Behalf Of > Yanan Wang > Sent: Sunday, May 16, 2021 11:32 AM > To: Peter Maydell <peter.maydell(a)linaro.org>; Paolo Bonzini > <pbonzini(a)redhat.com>; Andrew Jones <drjones(a)redhat.com>; Michael S . Tsirkin > <mst(a)redhat.com>; Igor Mammedov <imammedo(a)redhat.com>; Shannon Zhao > <shannon.zhaosl(a)gmail.com>; qemu-devel(a)nongnu.org; qemu-arm(a)nongnu.org > Cc: Song Bao Hua (Barry Song) <song.bao.hua(a)hisilicon.com>; Philippe > Mathieu-Daudé <philmd(a)redhat.com>; wangyanan (Y) <wangyanan55(a)huawei.com>; > Zengtao (B) <prime.zeng(a)hisilicon.com>; Wanghaibin (D) > <wanghaibin.wang(a)huawei.com>; yuzenghui <yuzenghui(a)huawei.com>; yangyicong > <yangyicong(a)huawei.com>; zhukeqian <zhukeqian1(a)huawei.com> > Subject: [RFC PATCH v3 4/4] hw/arm/virt: Parse -smp cluster parameter in > virt_smp_parse > > There is a separate function virt_smp_parse() in hw/virt/arm.c used > to parse cpu topology for the ARM machines. So add parsing of -smp > cluster parameter in it, then total number of logical cpus will be > calculated like: max_cpus = sockets * clusters * cores * threads. > > Note, we will assume multi-cluster in one socket is not supported > and default the value of clusters to 1, if it's not explicitly > specified in -smp cmdline. > > Signed-off-by: Yanan Wang <wangyanan55(a)huawei.com> > --- > hw/arm/virt.c | 32 ++++++++++++++++++-------------- > 1 file changed, 18 insertions(+), 14 deletions(-) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index 7de822e491..678d5ef36c 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -2642,8 +2642,8 @@ static int virt_kvm_type(MachineState *ms, const char > *type_str) > * with the -smp cmdlines when parsing them. > * > * We require that at least one of cpus or maxcpus must be provided. > - * Threads will default to 1 if not provided. Sockets and cores must > - * be either both provided or both not. > + * Clusters and threads will default to 1 if they are not provided. > + * Sockets and cores must be either both provided or both not. > * > * Note, if neither sockets nor cores are specified, we will calculate > * all the missing values just like smp_parse() does, but will disable > @@ -2652,15 +2652,18 @@ static int virt_kvm_type(MachineState *ms, const char > *type_str) > static void virt_smp_parse(MachineState *ms, QemuOpts *opts) > { > VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(ms); > + VirtMachineState *vms = VIRT_MACHINE(ms); > > if (opts) { > unsigned cpus = qemu_opt_get_number(opts, "cpus", 0); > unsigned maxcpus = qemu_opt_get_number(opts, "maxcpus", 0); > unsigned sockets = qemu_opt_get_number(opts, "sockets", 0); > + unsigned clusters = qemu_opt_get_number(opts, "clusters", 0); > unsigned cores = qemu_opt_get_number(opts, "cores", 0); > unsigned threads = qemu_opt_get_number(opts, "threads", 0); > > - /* Default threads to 1 if not provided */ > + /* Default clusters and threads to 1 if not provided */ > + clusters = clusters > 0 ? clusters : 1; > threads = threads > 0 ? threads : 1; > > if (cpus == 0 && maxcpus == 0) { > @@ -2676,13 +2679,13 @@ static void virt_smp_parse(MachineState *ms, QemuOpts > *opts) > cores = 1; > if (cpus == 0) { > sockets = 1; > - cpus = sockets * cores * threads; > + cpus = sockets * clusters * cores * threads; > } else { > maxcpus = maxcpus > 0 ? maxcpus : cpus; > - sockets = maxcpus / (cores * threads); > + sockets = maxcpus / (clusters * cores * threads); > } > } else if (sockets > 0 && cores > 0) { > - cpus = cpus > 0 ? cpus : sockets * cores * threads; > + cpus = cpus > 0 ? cpus : sockets * clusters * cores * threads; > maxcpus = maxcpus > 0 ? maxcpus : cpus; > } else { > error_report("sockets and cores must be both provided " > @@ -2695,25 +2698,26 @@ static void virt_smp_parse(MachineState *ms, QemuOpts > *opts) > exit(1); > } > > - if (sockets * cores * threads < cpus) { > + if (sockets * clusters * cores * threads < cpus) { > error_report("cpu topology: " > - "sockets (%u) * cores (%u) * threads (%u) < " > - "smp_cpus (%u)", > - sockets, cores, threads, cpus); > + "sockets (%u) * clusters (%u) * cores (%u) * " > + "threads (%u) < smp_cpus (%u)", > + sockets, clusters, cores, threads, cpus); > exit(1); > } > > - if (sockets * cores * threads != maxcpus) { > + if (sockets * clusters * cores * threads != maxcpus) { > error_report("cpu topology: " > - "sockets (%u) * cores (%u) * threads (%u) " > - "!= maxcpus (%u)", > - sockets, cores, threads, maxcpus); > + "sockets (%u) * clusters (%u) * cores (%u) * " > + "threads (%u) != maxcpus (%u)", > + sockets, clusters, cores, threads, maxcpus); > exit(1); > } > > ms->smp.cpus = cpus; > ms->smp.max_cpus = maxcpus; > ms->smp.sockets = sockets; > + vms->smp_clusters = clusters; This variable naming *smp_clusters* looks out-of-sorts. I thought a similar variable *smp_cpus* was destined to be removed for the reason given in below link - a patch by Andrew Jones? Link: https://lists.gnu.org/archive/html/qemu-arm/2020-12/msg00418.html Am I missing anything here? Salil. > ms->smp.cores = cores; > ms->smp.threads = threads; > }

1 0

[PATCH 8/9] tty: hvc_console: Remove the repeated words 'no' and 'from'
by Xiaofei Tan 17 May '21

17 May '21

Remove the repeated words 'no' and 'from', reported by checkpatch.pl. Signed-off-by: Xiaofei Tan <tanxiaofei(a)huawei.com> --- drivers/tty/hvc/hvc_console.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c index f31efeb..b6720b0 100644 --- a/drivers/tty/hvc/hvc_console.c +++ b/drivers/tty/hvc/hvc_console.c @@ -293,7 +293,7 @@ int hvc_instantiate(uint32_t vtermno, int index, const struct hv_ops *ops) if (vtermnos[index] != -1) return -1; - /* make sure no no tty has been registered in this index */ + /* make sure no tty has been registered in this index */ hp = hvc_get_by_index(index); if (hp) { tty_port_put(&hp->port); @@ -622,7 +622,7 @@ static u32 timeout = MIN_TIMEOUT; /* * Maximum number of bytes to get from the console driver if hvc_poll is * called from driver (and can't sleep). Any more than this and we break - * and start polling with khvcd. This value was derived from from an OpenBMC + * and start polling with khvcd. This value was derived from an OpenBMC * console with the OPAL driver that results in about 0.25ms interrupts off * latency. */ -- 2.8.1

1 0

[PATCH 7/9] tty: hvc_console: Add a blank line after declarations
by Xiaofei Tan 17 May '21

17 May '21

Add a blank line after declarations, reported by checkpatch.pl. Signed-off-by: Xiaofei Tan <tanxiaofei(a)huawei.com> --- drivers/tty/hvc/hvc_console.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c index a61cdf0..f31efeb 100644 --- a/drivers/tty/hvc/hvc_console.c +++ b/drivers/tty/hvc/hvc_console.c @@ -922,6 +922,7 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data, /* We wait until a driver actually comes along */ if (atomic_inc_not_zero(&hvc_needs_init)) { int err = hvc_init(); + if (err) return ERR_PTR(err); } -- 2.8.1

1 0

[PATCH 5/9] tty: hvc_console: Delete spaces prohibited around open parenthesis '(' and ')'
by Xiaofei Tan 17 May '21

17 May '21

Delete spaces prohibited after that open parenthesis '(' and before that close parenthesis ')', reported by checkpatch.pl. Signed-off-by: Xiaofei Tan <tanxiaofei(a)huawei.com> --- drivers/tty/hvc/hvc_console.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c index 2e5c133..39018e5 100644 --- a/drivers/tty/hvc/hvc_console.c +++ b/drivers/tty/hvc/hvc_console.c @@ -689,7 +689,7 @@ static int __hvc_poll(struct hvc_struct *hp, bool may_sleep) spin_unlock_irqrestore(&hp->lock, flags); tty_hangup(tty); spin_lock_irqsave(&hp->lock, flags); - } else if ( n == -EAGAIN ) { + } else if (n == -EAGAIN) { /* * Some back-ends can only ensure a certain min * num of bytes read, which may be > 'count'. -- 2.8.1

1 0

[PATCH 3/9] tty: hvc_console: Remove trailing whitespace
by Xiaofei Tan 17 May '21

17 May '21

Remove trailing whitespace, reported by checkpatch.pl. Signed-off-by: Xiaofei Tan <tanxiaofei(a)huawei.com> --- drivers/tty/hvc/hvc_console.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c index ddf07ff..13f63d5 100644 --- a/drivers/tty/hvc/hvc_console.c +++ b/drivers/tty/hvc/hvc_console.c @@ -204,7 +204,7 @@ static struct tty_driver *hvc_console_device(struct console *c, int *index) } static int hvc_console_setup(struct console *co, char *options) -{ +{ if (co->index < 0 || co->index >= MAX_NR_HVC_CONSOLES) return -ENODEV; -- 2.8.1

1 0

[PATCH 2/9] tty: hvc_console: Fix "foo * bar" should be "foo *bar"
by Xiaofei Tan 17 May '21

17 May '21

Fix "foo * bar" should be "foo *bar", reported by checkpatch.pl. Signed-off-by: Xiaofei Tan <tanxiaofei(a)huawei.com> --- drivers/tty/hvc/hvc_console.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c index a1eca9d..ddf07ff 100644 --- a/drivers/tty/hvc/hvc_console.c +++ b/drivers/tty/hvc/hvc_console.c @@ -344,7 +344,7 @@ static int hvc_install(struct tty_driver *driver, struct tty_struct *tty) * The TTY interface won't be used until after the vio layer has exposed the vty * adapter to the kernel. */ -static int hvc_open(struct tty_struct *tty, struct file * filp) +static int hvc_open(struct tty_struct *tty, struct file *filp) { struct hvc_struct *hp = tty->driver_data; unsigned long flags; @@ -386,7 +386,7 @@ static int hvc_open(struct tty_struct *tty, struct file * filp) return rc; } -static void hvc_close(struct tty_struct *tty, struct file * filp) +static void hvc_close(struct tty_struct *tty, struct file *filp) { struct hvc_struct *hp = tty->driver_data; unsigned long flags; -- 2.8.1

1 0

[PATCH 1/9] tty: hvc_console: Fix spaces required around that '='
by Xiaofei Tan 17 May '21

17 May '21

Fix spaces required around that '=', reported by checkpatch.pl. Signed-off-by: Xiaofei Tan <tanxiaofei(a)huawei.com> --- drivers/tty/hvc/hvc_console.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c index d0f0253..a1eca9d 100644 --- a/drivers/tty/hvc/hvc_console.c +++ b/drivers/tty/hvc/hvc_console.c @@ -944,7 +944,7 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data, * find index to use: * see if this vterm id matches one registered for console. */ - for (i=0; i < MAX_NR_HVC_CONSOLES; i++) + for (i = 0; i < MAX_NR_HVC_CONSOLES; i++) if (vtermnos[i] == hp->vtermno && cons_ops[i] == hp->ops) break; -- 2.8.1

1 0

Re: [PATCH net v8 1/3] net: sched: fix packet stuck problem for lockless qdisc
by Cong Wang 15 May '21

15 May '21

On Thu, May 13, 2021 at 7:27 PM Yunsheng Lin <linyunsheng(a)huawei.com> wrote: > struct qdisc_size_table { > @@ -159,8 +160,33 @@ static inline bool qdisc_is_empty(const struct Qdisc *qdisc) > static inline bool qdisc_run_begin(struct Qdisc *qdisc) > { > if (qdisc->flags & TCQ_F_NOLOCK) { > + if (spin_trylock(&qdisc->seqlock)) > + goto nolock_empty; > + > + /* If the MISSED flag is set, it means other thread has > + * set the MISSED flag before second spin_trylock(), so > + * we can return false here to avoid multi cpus doing > + * the set_bit() and second spin_trylock() concurrently. > + */ > + if (test_bit(__QDISC_STATE_MISSED, &qdisc->state)) > + return false; > + > + /* Set the MISSED flag before the second spin_trylock(), > + * if the second spin_trylock() return false, it means > + * other cpu holding the lock will do dequeuing for us > + * or it will see the MISSED flag set after releasing > + * lock and reschedule the net_tx_action() to do the > + * dequeuing. > + */ > + set_bit(__QDISC_STATE_MISSED, &qdisc->state); > + > + /* Retry again in case other CPU may not see the new flag > + * after it releases the lock at the end of qdisc_run_end(). > + */ > if (!spin_trylock(&qdisc->seqlock)) > return false; > + > +nolock_empty: > WRITE_ONCE(qdisc->empty, false); > } else if (qdisc_is_running(qdisc)) { > return false; > @@ -176,8 +202,15 @@ static inline bool qdisc_run_begin(struct Qdisc *qdisc) > static inline void qdisc_run_end(struct Qdisc *qdisc) > { > write_seqcount_end(&qdisc->running); > - if (qdisc->flags & TCQ_F_NOLOCK) > + if (qdisc->flags & TCQ_F_NOLOCK) { > spin_unlock(&qdisc->seqlock); > + > + if (unlikely(test_bit(__QDISC_STATE_MISSED, > + &qdisc->state))) { > + clear_bit(__QDISC_STATE_MISSED, &qdisc->state); We have test_and_clear_bit() which is atomic, test_bit()+clear_bit() is not. > + __netif_schedule(qdisc); > + } > + } > } > > static inline bool qdisc_may_bulk(const struct Qdisc *qdisc) > diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c > index 44991ea..795d986 100644 > --- a/net/sched/sch_generic.c > +++ b/net/sched/sch_generic.c > @@ -640,8 +640,10 @@ static struct sk_buff *pfifo_fast_dequeue(struct Qdisc *qdisc) > { > struct pfifo_fast_priv *priv = qdisc_priv(qdisc); > struct sk_buff *skb = NULL; > + bool need_retry = true; > int band; > > +retry: > for (band = 0; band < PFIFO_FAST_BANDS && !skb; band++) { > struct skb_array *q = band2list(priv, band); > > @@ -652,6 +654,23 @@ static struct sk_buff *pfifo_fast_dequeue(struct Qdisc *qdisc) > } > if (likely(skb)) { > qdisc_update_stats_at_dequeue(qdisc, skb); > + } else if (need_retry && > + test_bit(__QDISC_STATE_MISSED, &qdisc->state)) { > + /* Delay clearing the STATE_MISSED here to reduce > + * the overhead of the second spin_trylock() in > + * qdisc_run_begin() and __netif_schedule() calling > + * in qdisc_run_end(). > + */ > + clear_bit(__QDISC_STATE_MISSED, &qdisc->state); Ditto. > + > + /* Make sure dequeuing happens after clearing > + * STATE_MISSED. > + */ > + smp_mb__after_atomic(); > + > + need_retry = false; > + > + goto retry; Two concurrent pfifo_fast_dequeue() would possibly retry it at the same time when they test __QDISC_STATE_MISSED at the same time and get true. Is this a problem? Also, any reason why you want pfifo_fast to handle a generic Qdisc flag? IOW, why not handle this logic in, for example, qdisc_restart()? Thanks.

2 3

Re: [PATCH net v8 0/3] fix packet stuck problem for lockless qdisc
by patchwork-bot+netdevbpf＠kernel.org 15 May '21

15 May '21

Hello: This series was applied to netdev/net.git (refs/heads/master): On Fri, 14 May 2021 11:16:58 +0800 you wrote: > This patchset fixes the packet stuck problem mentioned in [1]. > > Patch 1: Add STATE_MISSED flag to fix packet stuck problem. > Patch 2: Fix a tx_action rescheduling problem after STATE_MISSED > flag is added in patch 1. > Patch 3: Fix the significantly higher CPU consumption problem when > multiple threads are competing on a saturated outgoing > device. > > [...] Here is the summary with links: - [net,v8,1/3] net: sched: fix packet stuck problem for lockless qdisc https://git.kernel.org/netdev/net/c/a90c57f2cedd - [net,v8,2/3] net: sched: fix tx action rescheduling issue during deactivation https://git.kernel.org/netdev/net/c/102b55ee92f9 - [net,v8,3/3] net: sched: fix tx action reschedule issue with stopped queue https://git.kernel.org/netdev/net/c/dcad9ee9e066 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html

1 0