Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 13 May 2021 21:59:46 -0400
From:   Daniel Jordan <daniel.m.jordan@oracle.com>
To:     Peter Zijlstra <peterz@infradead.org>,
        Huang Ying <ying.huang@intel.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org,
        Dan Carpenter <dan.carpenter@oracle.com>,
        Andrea Parri <andrea.parri@amarulasolutions.com>,
        Andi Kleen <ak@linux.intel.com>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Omar Sandoval <osandov@fb.com>,
        Paul McKenney <paulmck@kernel.org>, Tejun Heo <tj@kernel.org>,
        Will Deacon <will.deacon@arm.com>,
        Miaohe Lin <linmiaohe@huawei.com>
Subject: Re: [PATCH] mm, swap: Remove unnecessary smp_rmb() in
 swap_type_to_swap_info()
Message-ID: <20210514015946.nivgnoobef4nqwmw@oracle.com>
References: <20210513064837.3949064-1-ying.huang@intel.com>
 <YJ0fkhuAZ7JnNmEw@hirez.programming.kicks-ass.net>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YJ0fkhuAZ7JnNmEw@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
Received: from oracle.com (98.229.125.203) by BLAPR03CA0068.namprd03.prod.outlook.com (2603:10b6:208:329::13) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4108.25 via Frontend Transport; Fri, 14 May 2021 01:59:49 +0000
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id: e0851b4f-0e8f-4261-ef07-08d9167bf5ac
X-MS-TrafficTypeDiagnostic: BYAPR10MB3605:
X-MS-Exchange-Transport-Forked: True
X-Microsoft-Antispam-PRVS: <BYAPR10MB3605C153734C974B04DF45CED9509@BYAPR10MB3605.namprd10.prod.outlook.com>
X-MS-Oob-TLC-OOBClassifiers: OLM:10000;
X-MS-Exchange-SenderADCheck: 1
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: 1ZAMw5JER14TtQD6IflW1owQex0XXeXJBXqxvOCLjoX9UnRaJIrZsZ2qnpWgXGgthCc4kY9UTvenUXnqgrcHbgCfMIrK3LPe6rXMY7luY8xlZWykPkXfCsClZhPHEzWjdL2qS2LTTXYTAie2097Jr9M7BjmDf3PfQ+hg3QweEFKXjppx0lKMZhT+mc2+n8wWc+c4HFbxzMuDsWDGdM1gXl3nFUq7LPuDGOwNfyScA7WZNlZBJKJKctTdtm5zR/DU8psuX5h8q4RGj3RXhuizecBWiXOgPHGuCe3uVC3rED8P+IeLZ2fhH6I9SW/G+VaDrUHUe5l+WSvyN8+xgRsB7gEKDzJFp22VkFX5kg78Xc2va5kfLf95XTo6QUXqRtrfhdOrsSc6QP7sivPhGRr03merazbB3Fhwr8n9gnJMmz3r5x2TityXMFAegMoP4lKrWZHRQILie6qYQL8CgZEmWG1dK7P5IFCgDRHK586Osw+7qqrKUQM2irxP6DTm7KJGDnzxA21kNymFkmdKjvS0scFRJJ0BxlajI97Mko2K+3can8vyFFd5jDsJ/9tylv+QkdEh5LITyvWopqxBY4VMrwji2nysi9FW5zMg/wrcYT6c5z4YtlJ6glUVybmnZlLrirhHI/iOaj+7BWhJ+I3qBQ==
X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:BYAPR10MB2966.namprd10.prod.outlook.com;PTR:;CAT:NONE;SFS:(366004)(39860400002)(376002)(396003)(346002)(136003)(38350700002)(36756003)(38100700002)(4326008)(16526019)(55016002)(186003)(86362001)(83380400001)(7416002)(316002)(54906003)(8936002)(956004)(8676002)(1076003)(5660300002)(8886007)(2616005)(110136005)(6666004)(2906002)(7696005)(52116002)(66556008)(66476007)(66946007)(26005)(478600001);DIR:OUT;SFP:1101;
X-MS-Exchange-AntiSpam-MessageData: =?us-ascii?Q?xeHSQU+p6yBXtt6e1k6ak8FdP+aKKAVkvVxmW81T59Iq6V1Z6kaAywy+Od7T?=
 =?us-ascii?Q?idWjyLda3xMNwJ9FCvFZqDVPXPwQhBhJ7MfsbEMwW8GxllwQPgjDYndIHM0I?=
 =?us-ascii?Q?fNUxFxvbIVztyqt3N3X4AKSotWRGUCfdEtztKoN8zWPO+dcuQBS1yyruZLqM?=
 =?us-ascii?Q?j/wlM5UpmoWGQbfEqMdmh0wPwbDUOGNwfeAPb24IWLHSo9l/TLjW6g4vjCdx?=
 =?us-ascii?Q?ukjzBGC2Tzj12x5OtwAKfvnc4AZc/0EmsNRB9A9TCAxSvdpMtOts2Kr1ar6R?=
 =?us-ascii?Q?Xr1Rp5BumWz6HJZvqSYhTyClJtd0nlLo5KaXf3VyGmbVG/yyW8oUNGA0PM1Y?=
 =?us-ascii?Q?SNPeAsOtK3YkPeyCZ7HwzA2mS/4gNwOo8X0zLJ3rrUEcZpUjex2kqc/zzrUo?=
 =?us-ascii?Q?FKuX8lbiObUCTOjD4jto1J5ax8MolEt+X59wCRUxBOSXm3sp5LhdO9furvSa?=
 =?us-ascii?Q?QIq4wceLlr1X+aCcHaklTyjthZzpl4eWXHWuNhi/uYn+0e4Fv0j/iFPJ9HJZ?=
 =?us-ascii?Q?gZ9pe02gcjcY1adp5aTjHHz7VBM/89jRA/eUVpxYDDVfjMrja1+VKwUQTkhD?=
 =?us-ascii?Q?1VtOPbfWvHnHAQQAWNTMAZgD2DqWsm6ATqKos5ahYKb3LOdJB2HYXJAgaaQr?=
 =?us-ascii?Q?y/Z0ZimU+JuUhjJQJE3suilnjpJjNiRCn5g08n+gK3W4SJ3NXL5/8KlrOvTV?=
 =?us-ascii?Q?5Iby/KEcdRDRxy9j24F0YKaVK6fyvSgGnW65Ge+1VmidPUTpgRu8QVwh1nsQ?=
 =?us-ascii?Q?yUj3DlFFLO4f04mAvWkU3IB4RV8T0NZmrFp2ldhx5YqYYW/sKx/uN6v+snSV?=
 =?us-ascii?Q?AzesYSqE8LP2iAnvgjY+ZthWOzgCkCA6yzqXc2mrUCk4fqC+tjNhU1lF4f5D?=
 =?us-ascii?Q?PjLXirKPy3yW7gPA1vDm7qP0IaWuhifS1S7r/o341/ySmPTTO/6sDERFSEX4?=
 =?us-ascii?Q?vH/F2VDZ5w54CD7m6TZa+WflwihcbhY6hbaXsdaZju5a5F0AaZzKQ4TK5BLS?=
 =?us-ascii?Q?7xPQmm6gNFRY+1XyOpuj52C6LsPo1vW0O9OkbLHuBUnZ5gGJ/FpeMuHr+WoY?=
 =?us-ascii?Q?7A2ZEfXMB2kquzT0WN+sxaWTIUhn1A32tlwJwjrI/En8D9Jxlg4nxZzetfiE?=
 =?us-ascii?Q?XKi1a5rjs2Lpw69ybnz/RtuwLBsyjTcehBgmPgxnUtFziRyTDHy1prIUp9oh?=
 =?us-ascii?Q?MJn/rK5kpcnL1EkscvUPFnnlZlHH+sio21eOScvD9RLqUGypQPd5SIfckaK6?=
 =?us-ascii?Q?Dw4z6BLGzKaHEutqfIn1ILSJ2yrz7YwQDgsoYAUoYBnn7SiyoHz0hKQkj8CS?=
 =?us-ascii?Q?WOq7GKHp2JcjYSW28g7ZsPan?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e0851b4f-0e8f-4261-ef07-08d9167bf5ac
X-MS-Exchange-CrossTenant-AuthSource: BYAPR10MB2966.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 May 2021 01:59:51.3032
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 30yxSkpD6Lqo6dEpCO3N2a2ebSK3RAxX+yNO+DwOGdzxbCU0F3sEjl2zFoz5PCdScURbTnIrfjRMt3r47J4Sfib9uxs2TNLOWOmw2l7tdSg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB3605
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9983 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 suspectscore=0
 malwarescore=0 spamscore=0 mlxscore=0 adultscore=0 bulkscore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2104190000 definitions=main-2105140011
X-Proofpoint-ORIG-GUID: NOsE_sSOrsZRvLWsmJ2llwNGEr2_RIOv
X-Proofpoint-GUID: NOsE_sSOrsZRvLWsmJ2llwNGEr2_RIOv
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9983 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 impostorscore=0 phishscore=0
 priorityscore=1501 suspectscore=0 spamscore=0 lowpriorityscore=0
 adultscore=0 clxscore=1011 mlxscore=0 bulkscore=0 mlxlogscore=999
 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2104190000 definitions=main-2105140010
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, May 13, 2021 at 02:46:10PM +0200, Peter Zijlstra wrote:
> 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++;

Yes, this does help, I didn't understand why smp_wmb stayed around in
the original post.

I think the only access smp_store_release() orders is p->type.  Wouldn't
it be kinda inconsistent to only initialize that one field before
publishing when many others would be done at the end of
alloc_swap_info() after the fact?  p->type doesn't seem special.  For
instance, get_swap_page_of_type() touches si->lock soon after it calls
swap_type_to_swap_info(), so there could be a small window where there's
a non-NULL si with an uninitialized lock.

It's not as if this is likely to be a problem in practice, it would just
make it harder to understand why smp_store_release is there.  Maybe all
we need is a WRITE_ONCE, or if it's really necessary for certain fields
to be set before publication then move them up and explain?