On Wed, Jan 30, 2019 at 10:13:16AM +0100, Peter Zijlstra wrote:
> On Mon, Jan 14, 2019 at 07:23:05PM -0500, Daniel Jordan wrote:
>
> A few comments below, but:
>
> Acked-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
Thanks.
> > @@ -2799,9 +2810,9 @@ static void *swap_start(struct seq_file *swap, loff_t *pos)
> > if (!l)
> > return SEQ_START_TOKEN;
> >
> > - for (type = 0; type < nr_swapfiles; type++) {
> > + for (type = 0; type < READ_ONCE(nr_swapfiles); type++) {
> > smp_rmb(); /* read nr_swapfiles before swap_info[type] */
> > - si = swap_info[type];
> > + si = READ_ONCE(swap_info[type]);
> > if (!(si->flags & SWP_USED) || !si->swap_map)
> > continue;
> > if (!--l)
> > @@ -2821,9 +2832,9 @@ static void *swap_next(struct seq_file *swap, void *v, loff_t *pos)
> > else
> > type = si->type + 1;
> >
> > - for (; type < nr_swapfiles; type++) {
> > + for (; type < READ_ONCE(nr_swapfiles); type++) {
> > smp_rmb(); /* read nr_swapfiles before swap_info[type] */
> > - si = swap_info[type];
> > + si = READ_ONCE(swap_info[type]);
> > if (!(si->flags & SWP_USED) || !si->swap_map)
> > continue;
> > ++*pos;
>
> You could write those like:
>
> for (; (si = swap_type_to_swap_info(type)); type++)
That's clever, and way better than the ugly iterator macro I wrote and then
deleted in disgust.
> > @@ -2930,14 +2941,14 @@ static struct swap_info_struct *alloc_swap_info(void)
> > }
> > if (type >= nr_swapfiles) {
> > p->type = type;
> > - swap_info[type] = p;
> > + WRITE_ONCE(swap_info[type], p);
> > /*
> > * Write swap_info[type] before nr_swapfiles, in case a
> > * racing procfs swap_start() or swap_next() is reading them.
> > * (We never shrink nr_swapfiles, we never free this entry.)
> > */
> > smp_wmb();
> > - nr_swapfiles++;
> > + WRITE_ONCE(nr_swapfiles, nr_swapfiles + 1);
> > } else {
> > kvfree(p);
> > p = swap_info[type];
>
> It is also possible to write this with smp_load_acquire() /
> smp_store_release(). ARM64/RISC-V might benefit from that, OTOH ARM
> won't like that much.
>
> Dunno what would be better.
I just left it as-is for now.
