Messages in this thread Patch in this message | | | Date | Thu, 2 Mar 2023 18:16:42 -0300 | From | Marcelo Tosatti <> | Subject | Re: [PATCH v2 10/11] mm/vmstat: switch vmstat shepherd to flush per-CPU counters remotely |
| |
On Thu, Mar 02, 2023 at 04:01:07PM -0500, Peter Xu wrote: > On Thu, Feb 09, 2023 at 12:02:00PM -0300, Marcelo Tosatti wrote: > > +#ifdef CONFIG_HAVE_CMPXCHG_LOCAL > > +/* Flush counters remotely if CPU uses cmpxchg to update its per-CPU counters */ > > +static void vmstat_shepherd(struct work_struct *w) > > +{ > > + int cpu; > > + > > + cpus_read_lock(); > > + for_each_online_cpu(cpu) { > > + cpu_vm_stats_fold(cpu); > > Nitpick: IIUC this line is the only change with CONFIG_HAVE_CMPXCHG_LOCAL > to replace the queuing. Would it be cleaner to move the ifdef into > vmstat_shepherd, then, and keep the common logic?
https://lore.kernel.org/lkml/20221223144150.GA79369@lothringen/
Could have
#ifdef CONFIG_HAVE_CMPXCHG_LOCAL int cpu_flush_vm_stats(int cpu) { return cpu_vm_stats_fold(cpu); } #else int cpu_flush_vm_stats(int cpu) { struct delayed_work *dw = &per_cpu(vmstat_work, cpu);
if (!delayed_work_pending(dw) && need_update(cpu)) queue_delayed_work_on(cpu, mm_percpu_wq, dw, 0); } #endif
static void vmstat_shepherd(struct work_struct *w) { int cpu;
cpus_read_lock(); for_each_online_cpu(cpu) { cpu_flush_vm_stats(cpu); cond_resched(); } cpus_read_unlock();
schedule_delayed_work(&shepherd, round_jiffies_relative(sysctl_stat_interval)); }
This looks really awkward to me. But then, we don't want schedule_delayed_work if !CONFIG_HAVE_CMPXCHG_LOCAL. The common part would be the cpus_read_lock and for_each_online_cpu loop.
So it seems the current separation is quite readable (unless you have a suggestion).
> > + cond_resched(); > > + } > > + cpus_read_unlock(); > > + > > + schedule_delayed_work(&shepherd, > > + round_jiffies_relative(sysctl_stat_interval)); > > +} > > +#else > > static void vmstat_shepherd(struct work_struct *w) > > { > > int cpu; > > @@ -2026,6 +2043,7 @@ static void vmstat_shepherd(struct work_ > > schedule_delayed_work(&shepherd, > > round_jiffies_relative(sysctl_stat_interval)); > > } > > +#endif > > > > static void __init start_shepherd_timer(void) > > { > > > > > > > > -- > Peter Xu > >
| |