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
>
>
