Peter Zijlstra <peterz@xxxxxxxxxxxxx> writes:
> On Thu, May 13, 2021 at 02:48:37PM +0800, Huang Ying wrote:
>> mm/swapfile.c | 18 +++++++++---------
>> 1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/mm/swapfile.c b/mm/swapfile.c
>> index 2aad85751991..4c1fb28bbe0e 100644
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -100,10 +100,14 @@ atomic_t nr_rotate_swap = ATOMIC_INIT(0);
>>
>> static struct swap_info_struct *swap_type_to_swap_info(int type)
>> {
>> - if (type >= READ_ONCE(nr_swapfiles))
>> + if (type >= MAX_SWAPFILES)
>> return NULL;
>>
>> - smp_rmb(); /* Pairs with smp_wmb in alloc_swap_info. */
>> + /*
>> + * The data dependency ordering from the READ_ONCE() pairs
>> + * with smp_wmb() in alloc_swap_info() to guarantee the
>> + * swap_info_struct fields are read after swap_info[type].
>> + */
>> return READ_ONCE(swap_info[type]);
>> }
>>
>> @@ -2884,14 +2888,10 @@ static struct swap_info_struct *alloc_swap_info(void)
>> }
>> if (type >= nr_swapfiles) {
>> p->type = type;
>> - 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.)
>> - */
>> + /* Paired with READ_ONCE() in swap_type_to_swap_info() */
>> smp_wmb();
>> - WRITE_ONCE(nr_swapfiles, nr_swapfiles + 1);
>> + WRITE_ONCE(swap_info[type], p);
>> + nr_swapfiles++;
>
> Ah, I think I see what you meant to say, it would perhaps help if you
> write it like so:
>
>
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 149e77454e3c..94735248dcd2 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -99,11 +99,10 @@ atomic_t nr_rotate_swap = ATOMIC_INIT(0);
>
> static struct swap_info_struct *swap_type_to_swap_info(int type)
> {
> - if (type >= READ_ONCE(nr_swapfiles))
> + if (type >= MAX_SWAPFILES)
> return NULL;
>
> - smp_rmb(); /* Pairs with smp_wmb in alloc_swap_info. */
> - return READ_ONCE(swap_info[type]);
> + return READ_ONCE(swap_info[type]); /* rcu_dereference() */
> }
>
> static inline unsigned char swap_count(unsigned char ent)
> @@ -2869,14 +2868,11 @@ static struct swap_info_struct *alloc_swap_info(void)
> }
> if (type >= nr_swapfiles) {
> p->type = type;
> - 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.)
> + * Publish the swap_info_struct.
> */
> - smp_wmb();
> - WRITE_ONCE(nr_swapfiles, nr_swapfiles + 1);
> + smp_store_release(&swap_info[type], p); /* rcu_assign_pointer() */
> + nr_swapfiles++;
> } else {
> defer = p;
> p = swap_info[type];
OK. It seems that this helps people to understand. I will use this in
the next version.
Best Regards,
Huang, Ying
