Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 10 Feb 2022 12:03:12 -0800
From:   Roman Gushchin <guro@fb.com>
To:     Shakeel Butt <shakeelb@google.com>
CC:     Johannes Weiner <hannes@cmpxchg.org>,
        Michal Hocko <mhocko@suse.com>,
        Chris Down <chris@chrisdown.name>,
        Andrew Morton <akpm@linux-foundation.org>,
        <cgroups@vger.kernel.org>, <linux-mm@kvack.org>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/4] memcg: unify force charging conditions
Message-ID: <YgVvgCbbTrDPb5tT@carbon.dhcp.thefacebook.com>
References: <20220210081437.1884008-1-shakeelb@google.com>
 <20220210081437.1884008-3-shakeelb@google.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20220210081437.1884008-3-shakeelb@google.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?ms3MhJKZhToTEDTzStt3w2zb3YcWjOvrVLnovlPh3sShYpH9tRi/YZR/MlbD?=
 =?us-ascii?Q?EJI/oSjM99jLFcKJYUgl9afs8hy3U96bZvflZcJRuj8enR/FMtf0Ct4rDQVx?=
 =?us-ascii?Q?7IJngS4rnpsFyUBjw9rKrZHDIFyG76v/84/v0vowjF3DHHbKOgsH7msoZn/O?=
 =?us-ascii?Q?QJlhv5Sd83aDBHdtHKqzUdpqypTwkkQnPVi4bMMMZACD0oldXx36238zpF+8?=
 =?us-ascii?Q?IyhPXDh0//WCzhNWPxH1Bv6WDR+QIzy6ErBGNZ+lwZFjGAvhRpAFt6df8EP0?=
 =?us-ascii?Q?Ht6SKqPpH6sAAJoYuWTGTWmcHloscv5Xy0kt5nprvSR58WCQ1XFrz1UY4F2c?=
 =?us-ascii?Q?CCzJCSiXgnT1aTBEuvkkgGNVwnWtzoKspoPHmgR1XMGDcSjGuhyfezR46VPJ?=
 =?us-ascii?Q?y2IRmfoxkv81LUVujov3l7B63eeIcioObSi2qxYICtLfiqFMor9Nj0sL/MEG?=
 =?us-ascii?Q?FuWyAFboVaoIRg36a0r7SymcdzKTjg5DB8o9+NFoGlbMF9eRregP3DQkLF+q?=
 =?us-ascii?Q?gLOBwF6CgbMS55XJ1EqK1giQDooaONXDcOIVprtiLjO3cFqG91idNUZmG99z?=
 =?us-ascii?Q?cKsrgqSSZHQNqWS3sn98cJOcI6ba4a7IV2Y1yTE6BHne5qGqGYRE99VUtnyB?=
 =?us-ascii?Q?QQOXK7J/GjzNC7VIjM3T5FjfzTvlGVCnmYAwtQkgyAqclheDYvoC/5bWlE+C?=
 =?us-ascii?Q?BZEhGwi/l9ROZ+80fmdcsUDzvww3pY3MV1bISWPLPcQ5ThB3C6QsLst8Pc4J?=
 =?us-ascii?Q?oq9Wk36RClg111jU8JV2pko60pOcHJU8oKx5igC0LFpBiDqtd3ckb2MI4NZB?=
 =?us-ascii?Q?co0n36bWpnribXlX6f7B8rbV8wGFxwe/RFQiJlwRsDRpKhSsX0edERBeca3X?=
 =?us-ascii?Q?YePorc7g7y/xYEkJXCRLjGFNnkCSOvPjhmZI5r3kCrAjRCwMkSJys+HlyYVY?=
 =?us-ascii?Q?LqX6pYmtEDz8D4IgeXfB8epnp3LRBDGxTSKfPZKYFoPxeqnMUxe6Vf0gd1A2?=
 =?us-ascii?Q?cC+3dj3/eRhpUjNMaBi7khSTHtiMbxYTN8YoGQPFiKZ/mUk0o7BudUNYpOq/?=
 =?us-ascii?Q?gopKci/nCTmcMXEHvFSNXh5h1K0NsTryEAnSsK1LRcfR6vkgGVD5NaQt7hKA?=
 =?us-ascii?Q?WhWMLDQNnUK8JM3BcSOvi6fQ9vcAt0OEj6goFg1ezTIfOLVEa4HD9IKlGdC/?=
 =?us-ascii?Q?LeLhG6U+HMuLT91JP6FLXjU32eEcfX6fjBY2e4psJlUPOFYWXY8PN+m9y3sE?=
 =?us-ascii?Q?O5ROaxlR5QnRXJFLDNK3vQ5rCyqmdSj6F5W4lnNTmJMyXKrViKOXl4nK0Lgy?=
 =?us-ascii?Q?wh8RC1Ltkk7ZjhbWfxuKkrXQGQ5XxeD4UUz8x1HUYJHM4Qek9DkHLZPboS8a?=
 =?us-ascii?Q?smbM2yoHsVKp03YX4yq283qDcujCXKjDPMOyW5T0WHXMnsB2/IZFv4D+gbRn?=
 =?us-ascii?Q?VbSp+TSof1NYsQn7lCbBh2PAFFLWr8QaKT3WCvERiHTnu+o0Zy35jWdcPheg?=
 =?us-ascii?Q?dq9EBwS6P723miYHFRKvuN9KcooEBfrUcRc5GdtDwvbBjlUgx4rnYGA3UxWD?=
 =?us-ascii?Q?JtT2gDHM7MTBYeeRfg4NtfGIpHjeN52riFjOToFX?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 07cb144b-11a6-4a4a-4cce-08d9ecd06033
X-MS-Exchange-CrossTenant-AuthSource: BYAPR15MB4136.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Feb 2022 20:03:16.2369
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: OLQdlSJorMtqTaFlMwRicVZdST5aWp8aty9WLZKxPmNy2g6LQj5torTM4MrJ2Stv
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR15MB3534
X-OriginatorOrg: fb.com
X-Proofpoint-GUID: oFxxgnjyEfaRquUlDtuhRjBhtrx6jSzM
X-Proofpoint-ORIG-GUID: oFxxgnjyEfaRquUlDtuhRjBhtrx6jSzM
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.816,Hydra:6.0.425,FMLib:17.11.62.513
 definitions=2022-02-10_09,2022-02-09_01,2021-12-02_01
X-Proofpoint-Spam-Details: rule=fb_outbound_notspam policy=fb_outbound score=0 malwarescore=0
 suspectscore=0 bulkscore=0 mlxlogscore=999 clxscore=1015
 priorityscore=1501 lowpriorityscore=0 spamscore=0 impostorscore=0
 phishscore=0 adultscore=0 mlxscore=0 classifier=spam adjust=0 reason=mlx
 scancount=1 engine=8.12.0-2201110000 definitions=main-2202100105
X-FB-Internal: deliver
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Feb 10, 2022 at 12:14:35AM -0800, Shakeel Butt wrote:
> Currently the kernel force charges the allocations which have __GFP_HIGH
> flag without triggering the memory reclaim. __GFP_HIGH indicates that
> the caller is high priority and since commit 869712fd3de5 ("mm:
> memcontrol: fix network errors from failing __GFP_ATOMIC charges") the
> kernel let such allocations do force charging. Please note that
> __GFP_ATOMIC has been replaced by __GFP_HIGH.
> 
> __GFP_HIGH does not tell if the caller can block or can trigger reclaim.
> There are separate checks to determine that. So, there is no need to
> skip reclaim for __GFP_HIGH allocations. So, handle __GFP_HIGH together
> with __GFP_NOFAIL which also does force charging.

This sounds very reasonable. But shouldn't we check if __GFP_DIRECT_RECLAIM
is set and bail out otherwise?

Thanks!

> 
> Please note that this is a noop change as there are no __GFP_HIGH
> allocators in kernel which also have __GFP_ACCOUNT (or SLAB_ACCOUNT) and
> does not allow reclaim for now. The reason for this patch is to simplify
> the reasoning of the following patches.
> 
> Signed-off-by: Shakeel Butt <shakeelb@google.com>
> ---
>  mm/memcontrol.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index c40c27822802..ae73a40818b0 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2560,15 +2560,6 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask,
>  		goto retry;
>  	}
>  
> -	/*
> -	 * Memcg doesn't have a dedicated reserve for atomic
> -	 * allocations. But like the global atomic pool, we need to
> -	 * put the burden of reclaim on regular allocation requests
> -	 * and let these go through as privileged allocations.
> -	 */
> -	if (gfp_mask & __GFP_HIGH)
> -		goto force;
> -
>  	/*
>  	 * Prevent unbounded recursion when reclaim operations need to
>  	 * allocate memory. This might exceed the limits temporarily,
> @@ -2642,7 +2633,13 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask,
>  		goto retry;
>  	}
>  nomem:
> -	if (!(gfp_mask & __GFP_NOFAIL))
> +	/*
> +	 * Memcg doesn't have a dedicated reserve for atomic
> +	 * allocations. But like the global atomic pool, we need to
> +	 * put the burden of reclaim on regular allocation requests
> +	 * and let these go through as privileged allocations.
> +	 */
> +	if (!(gfp_mask & (__GFP_NOFAIL | __GFP_HIGH)))
>  		return -ENOMEM;
>  force:
>  	/*
> -- 
> 2.35.1.265.g69c8d7142f-goog
>