Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
Message-ID: <2593de09-22fb-5579-2022-c0fccff7f6b3@oracle.com>
Date:   Tue, 12 Dec 2023 13:48:56 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH] maple_tree: do not preallocate nodes for slot stores
Content-Language: en-US
To:     Matthew Wilcox <willy@infradead.org>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        maple-tree@lists.infradead.org, akpm@linux-foundation.org,
        liam.howlett@oracle.com, zhangpeng.00@bytedance.com
References: <20231212194640.217966-1-sidhartha.kumar@oracle.com>
 <ZXjJTM0pDksnslgo@casper.infradead.org>
From:   Sidhartha Kumar <sidhartha.kumar@oracle.com>
In-Reply-To: <ZXjJTM0pDksnslgo@casper.infradead.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MEE1R1g2RnFyY1A0NXYxVnJoZXgrZGdrckVCaFk3M3BDMlFOWEt5T2pLb1RF?=
 =?utf-8?B?VHJTN0QxL2FrMVp5V29EdGZxR2NzQjNUMTlWNCtCYVI5WW5TR3NvVGFSRWgw?=
 =?utf-8?B?ckZWcXVvekYyUTJCeFB0d20rbEYzWEJIZ2NkVWlhZkVVV3hCMFFmM0Z5b3ln?=
 =?utf-8?B?TVZ2eFRJeWNWbnJSTzZtc1NsR0MyQ0YvZUNNSjdKV0FVTmU3bGpWc1JDck5S?=
 =?utf-8?B?K3FDQ00wdytsamxkNHVDY2xuN0VVMlJYUG1zYjcrL0VSV3BETVNMQ3k5am92?=
 =?utf-8?B?UHFTTUh5SVR0VjlVeHo4RHlVUEVRV2FLbk9MVnZTdEVoMW10U1pWWSt0NmF3?=
 =?utf-8?B?ZnB4ZEVpWnN2SmplZlN0aUhLMnVpOUgwSno0ZnVWMDcvR3kwVy8yazZNa2pk?=
 =?utf-8?B?TE5heUhXcTFZMlVTOVlEOHZaUW1hYzIrQmt6cVQ4QW9QUUxpclVTb0pIaDBD?=
 =?utf-8?B?RDQzazRicjFWMlJ4TmRjOWNLdUJuL1FXUGFyU1RYaVc0WEhzNzNkK2ZqQldo?=
 =?utf-8?B?MS94WDh6c2ZHWUJxWWFjTS9XOW95clJBSlAxNzllSksxSHdVd3AyWlBrYUV1?=
 =?utf-8?B?bDB3WENvQ2ZjN2p3VG1NQ0xLbzJWSit4Qm5JUWU1c0t0VTB2c1hGUWM1OGRC?=
 =?utf-8?B?QmdnZ24vbXhOWVJ2am5ELy9IeHNFalBtZ1M5dVYyM3J5UlBqM0lqdjAzdTBh?=
 =?utf-8?B?NGROVHl0NVEvN1FLMFYyaDNvd2hWSGt0QmplbER2bjVaYjNjN0ZwN1R4dVF1?=
 =?utf-8?B?a1FtRDVHUTh5Y0VFM0VuOEVKSmxHdTFVR0VNTmhUYUdpY2pld0dwSXZoTGdG?=
 =?utf-8?B?YlE4djlCUlc0c2o3aHJUUlZTUFNnazU5RnhiNHlOQlJpSnByQXRSQVZOT3ZE?=
 =?utf-8?B?eTBpMUVhNUwwbkwxRUlFNXJRTmN2T2NvZzdVQ05rT0F3VS90aWl3eVRxLzg2?=
 =?utf-8?B?N1BVYnVma3pBc0M1cXZBYnJ1YWppRjJXNVFSTFdLeTI0ZEZ3dzNPUmJLc0Nn?=
 =?utf-8?B?YitibG5UcTZwWGFxcVBSZHFPQ3N3Z0VnVllTU1lkMzNtNytjWmZnSEc0NTh4?=
 =?utf-8?B?MkdvNVlTL2xQM1FPVVlNQjIxU0pqOHhUWGFRR0VQdkNVbG42cmVOdHg2amdr?=
 =?utf-8?B?Y1Y5UlUwazN0VHErU0NmRlBmM0NsSXRwKzQxZ3ZXelVjYWw5blNBMGdyMGpx?=
 =?utf-8?B?OEdKMnl2MlIvZGt2SEltUHg0eEc3WVlsSGhJTzRsVVVxaVpvUWUxaHNZdlZr?=
 =?utf-8?B?cXZFSTNWbnhwS2c0Sk1FYUtkbUxaOE5EYnI0MVVmeEo1VGdDL2dMaFVIT3dU?=
 =?utf-8?B?a2xXYm15WDJ0Y1BGSXV3VlZvakhSc1pwRTA1QmRwUG53QjZPbG5sQXIzWUkv?=
 =?utf-8?B?eU5HSHI0S1k0SXo1SzAwK3d2Y2t2dGR6cnd2TmxRZDRKRmU4REt5MUhFREZP?=
 =?utf-8?B?dFhzWGYzalNWc3VVU0cxbmcvVmFUQ3hFb1RjaXFGbE13WUsyQUlJbWdZYnMr?=
 =?utf-8?B?enZyOU03TnNDNnhET0RJQzRZNFlHSlNwWUMwSVZuNWw2MmZBVHRYbHNHaFBn?=
 =?utf-8?B?RmZtR2NadUVYYlNLaTFLYW5lVlB5ZmFPRUhqSnFyb2xVUnpsck13YXB4djIw?=
 =?utf-8?B?Q2dYaFg3bmp6ZFZqZEp5cjYrYkVVZjFQckoyYU4xV2lMdWR5YzdUWVVBRXdZ?=
 =?utf-8?B?WHJrdGJFVlh3ZjZzNjR2Yjg4ZGpMUmlycExKQjF4OW44dC9OeG0zS2R2NnhT?=
 =?utf-8?B?b0FuNGJxTVI4ZndsdXBmMHNFTUZWUmhBMHIwZTk1NmZpRXRkc0hGY1NhWldq?=
 =?utf-8?B?bFMvN3pZdUMvcDVvN2J4d0Q0c3RqdmZjZUpnR3dBU0ZmTU1SYnJ4MjJWYXFj?=
 =?utf-8?B?RGI1RWtuOXdqRTFxY1hQZSsrWkk1bkR1bFBHeGZ4ek42Qmt2WlpEUit5Mnky?=
 =?utf-8?B?dUdKMG9WeURWQm1IU2JwSWxselFGMFlKWFhuNTZqa1Z2djcrZXZWYUk1VkUx?=
 =?utf-8?B?dkdVRytLRmNJS1RwdllyMDNRU1NxL1F2cTFLUW0rYVFiSWtRSGh2MXRJRjFu?=
 =?utf-8?B?RU02aThURXV2YUtIbHRrTWljMWdKcnJ0dVl1UHVmZVRIYUFnYi8xZTJUcFQr?=
 =?utf-8?B?TG9rbEI2NUlxVytRbXJJMldqOE1laWc3WCtFTXZrRVllR0NIY0NxKzJ0eVlT?=
 =?utf-8?Q?MkeYMF0LgJ5A2lvFHR5dM+U=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: Z+A8Brkgty62gIoU/c76EnoGLNEmqSolO3SfNgwbuI5WxehI/r3qu4bWYKNIEoUOfGQ8fmxo8+lsotRR4bVbs9PFZZbJ1+K/Ejb1AsGwdloF2MZ5HrnQcInDYqWoyGmlXHPvbhbu3525GFfAZkjWx6qrrXlVHlJ/JuQZwCnpZJpHRFnwVtLIPTwj2pro3aHRQYN0vkyTDul7WqfukJVZOBzMUSKXolVwdR36dc/RXeSS12v/oD2in+5beSJ2Kb1pP0gsiL+BFumcNOWRVYQEeNt+3tvw6Yu+DmMd2XBdZQePOi3XOkLtRRGEJJfFVH+XnZpqCWIvfNxqeSIIuREJz1GqdVI2YyICKuV1G3y8ssyjrRZQg9H9DEA1IA/Iyu30oQH3tRjOF6z63A3QMZp6YHYv60ub7NjBE4s/fNCnew3nITKi1sFLkQzxXE2EAIf2d0Ne3tocHqqC8aAC9e/IfJpQB3DYDxdhYzNljwFwkAQuAArQN1mBv4qQKsr96khdoWcVSXteDLNJEOH36iU66fLdXNgbsM30Q2L5beaD1LsYKsttLfbOBHtrDWZOP7QyV6GazA10V0m3FkdkhXEEiZgDsIly6t6VsFk/GhOHTEc=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f399d7d4-dece-460b-f288-08dbfb5c25cf
X-MS-Exchange-CrossTenant-AuthSource: CH0PR10MB5113.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Dec 2023 21:48:59.5009
 (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: +h/krIeT2Kthy1k5m874IeD8Ay/k/78O53VUur+elnvfn/z3IUOFKMF+i4VXGDuWP5iMXk408BNpCf6GIkZvuq7Z6GiFJLDTcsuSs3mIDl4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN0PR10MB5175
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.997,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-12-12_12,2023-12-12_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 suspectscore=0
 phishscore=0 malwarescore=0 spamscore=0 mlxlogscore=999 bulkscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2312120169
X-Proofpoint-GUID: L1per04m4pjgDUTHGOqHEokoCtoSprTn
X-Proofpoint-ORIG-GUID: L1per04m4pjgDUTHGOqHEokoCtoSprTn
X-Spam-Status: No, score=-3.4 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on morse.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (morse.vger.email [0.0.0.0]); Tue, 12 Dec 2023 13:49:37 -0800 (PST)

On 12/12/23 12:57 PM, Matthew Wilcox wrote:
> On Tue, Dec 12, 2023 at 11:46:40AM -0800, Sidhartha Kumar wrote:
>> +	/* Slot store, does not require additional nodes */
>> +	if ((node_size == mas->end) && ((!mt_in_rcu(mas->tree))
>> +		|| (wr_mas.offset_end - mas->offset == 1)))
>> +		return 0;
> 
> Should we refactor this into a mas_is_slot_store() predicate?
  yes, I think we should add helper functions to identify the different type of 
stores. Thanks for the pointers to code style this is what I think the slot 
store identifying helper function would look like:

static inline bool mas_wr_is_slot_store(struct ma_wr_state *wr_mas)
{
	struct ma_state *mas = wr_mas->mas;
	unsigned char node_size = mas_wr_new_end(wr_mas);

	if ((node_size == mas->end) &&
	    (!mt_in_rcu(mas->tree) || (wr_mas->offset_end - mas->offset == 1)))
		return true;

	return false;
}

thanks,
Sid
> A few coding-style problems with it as it's currently written:
> 
> 1. The indentation on the second line is wrong.  It makes the
> continuation of the condition look like part of the statement.  Use
> extra whitespace to indent.  eg:
> 
> 	if ((node_size == mas->end) && ((!mt_in_rcu(mas->tree))
> 			|| (wr_mas.offset_end - mas->offset == 1)))
> 		return 0;
> 
> 2. The operator goes last on the line, not at the beginning of the
> continuation line.  ie:
> 
> 	if ((node_size == mas->end) && ((!mt_in_rcu(mas->tree)) ||
> 			(wr_mas.offset_end - mas->offset == 1)))
> 		return 0;
> 
> 3. You don't need parens around the !mt_in_rcu(mas->tree).  There's
> no ambiguity to solve here:
> 
> 	if ((node_size == mas->end) && (!mt_in_rcu(mas->tree) ||
> 			(wr_mas.offset_end - mas->offset == 1)))
> 		return 0;
> 
> But I'd write it as:
> 
> 	if ((node_size == mas->end) &&
> 	    (!mt_in_rcu(mas->tree) || (wr_mas.offset_end - mas->offset == 1)))
> 		return 0;
> 
> because then the whitespace matches how you're supposed to parse the
> condition, and so the next person to read this code will have an easier
> time of it.
>