Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Message-ID: <0473a930-9b80-4b81-81a1-c9adb0dc3919@intel.com>
Date:   Wed, 13 Dec 2023 09:47:55 -0700
User-Agent: Betterbird (Linux)
Subject: Re: [PATCH] nvdimm-btt: simplify code with the scope based resource
 management
To:     <dinghao.liu@zju.edu.cn>
CC:     Vishal Verma <vishal.l.verma@intel.com>,
        Dan Williams <dan.j.williams@intel.com>,
        Ira Weiny <ira.weiny@intel.com>, <nvdimm@lists.linux.dev>,
        <linux-kernel@vger.kernel.org>
References: <20231210102747.13545-1-dinghao.liu@zju.edu.cn>
 <8716bf72-e4ca-4f83-8a30-327baf459dbb@intel.com>
 <369f7d97.32dac.18c6129229a.Coremail.dinghao.liu@zju.edu.cn>
Content-Language: en-US
From:   Dave Jiang <dave.jiang@intel.com>
In-Reply-To: <369f7d97.32dac.18c6129229a.Coremail.dinghao.liu@zju.edu.cn>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?OHhoMVUxam5iWFBheVhvVE1FQm82THpmRy9wVHIyVlZQT3k0dEFyb21wNFFB?=
 =?utf-8?B?QTIyQXJkNE1VYnlLbWxudk53QnBSTnFzMThuYVFtVm42K1R3cWM0VTZDTDEr?=
 =?utf-8?B?Nlg3ZTJ2ZnQ2RTVVNFZySUFjeUt3RVdLcW51TlpBVCtkcU1NTlJUUXJTZ1Jh?=
 =?utf-8?B?NEFWOHg5OFI5VlFYOGpsM1RtRkRpcVpuWlNSZTNReEV2TmEvZ2xqNlorbFJV?=
 =?utf-8?B?c2gxTXBLeTRseWw3eWIvYkZRbUtNWkVDbEVvMGwyWjZoMnhVcTJBZklMc001?=
 =?utf-8?B?bFZGYUVxOFhHdG9WWldZUTA3VzFKaDFBUWM4RkkzZHFQS1NPbW1oZjN3Z3Ro?=
 =?utf-8?B?RndCM25pNFF2NTZrakw0Y3Z0V2JvUG0vZ04xc1RZK2U1Z0pEak1hd0dKWmRm?=
 =?utf-8?B?ZVcxb0wvVmQ3REg1eWFIaXNBRkwwYitTdFJuRlJ3L2VMa3N3d1FzMmRRYmRs?=
 =?utf-8?B?ZEs5QzNPWGRyQ3U2dFpjekJTbWh4YXdzR0VmT2hlUTFIY0VtTHcrL2pzb1lN?=
 =?utf-8?B?YXdTeU1XL3VyTHZWaG5pZHFLbE45RXk4OEdqbzFZZlovdXpQYTEwRzlRZWxF?=
 =?utf-8?B?VHhBWXY0OHhMcDRGd1hXQkRIWXN2Z0UyeVJ3bmpudTY1eXZXdHhidU14T2l2?=
 =?utf-8?B?djdlc0hReVBBOGp3dDZ0R01kSE54VExyajAraUd6TmsyUW5SeVBpazFQNyto?=
 =?utf-8?B?MUJPUHJmTUIwT3dhdDNJbExOaUVaTXZ0b05IajJpL29VZHZ6UUR1Ung1a2N3?=
 =?utf-8?B?V0JSTHBOVzRKZ3kyZ1JBcWhPNDlIT0M4UVdVSGcycENIczVBSDIyUDV2L1hM?=
 =?utf-8?B?QkowNW9OOHgzUGR3RW0xWjRiWDRuUTAxdE80UmhYVTZ1UjJYOG1IRllSdytQ?=
 =?utf-8?B?TFlUMC9RM09lQVhCbyt5a3g4ZlMvdmZGNmdTdzU1R2tBUGh5M3RlRTZkNGdD?=
 =?utf-8?B?elplSUJ3dzdkU2gyUTlpTWExRVZnejBTaHBlZ0o0amF5ZzE2QUwrL285RlhX?=
 =?utf-8?B?WGZ4NUFIOE5nOGlEVHFYcFNPbDl6STdXZS9wdWtucEFsUnI4RW5tSnVNVHZK?=
 =?utf-8?B?T2NsU255S3B5M3I5Qi8yblhJU2FMVUdjaW5pZlhJMlJFN2ZydFhnUTBBUElP?=
 =?utf-8?B?c05TS1pFaUxBOENHZVYwYmg1ckZpMW1mZmZiVXVQTUIzd1huMndtQm1ycHpy?=
 =?utf-8?B?ZzZvOWsyQlR5RDNQN1I5TStHcXlnT3NjakRCaERFSklaMTVyeGZadnNVQitI?=
 =?utf-8?B?UXhaWStLM2N2L29EUWxKUS84ZDNrRzhiQUpURTNOM2ZUZkN6L2Q1Q0dBYk9U?=
 =?utf-8?B?NWtzbWg1czNmNlFzVWN3NStCSGJFa2VwbDFtdGV0bHA5b2VwSUdEdGh5SVhn?=
 =?utf-8?B?c0dTY0diWkU4Rlc2Q09Vck5ueis0ckRlSm0yVEc1dkxWZnNHUmtuWUFwbkc5?=
 =?utf-8?B?T1VWYlJhM09ZT0VHU1drQnlkcnprMlk4TTlyWEtaWm82WHBESkhNRFdOcTk5?=
 =?utf-8?B?ZEQxcDVjTTRBak9NZXFkT2tpVjRUVGl2cFhvZlpRVThmU1ZQeFZTa2VMWnFa?=
 =?utf-8?B?eERiZjV3ZFRGNnBJNmcwNC8vVjY0NTlsa2hYMXd5YVNLUUN6WDloOVMrVjc3?=
 =?utf-8?B?UmtHYnRZc2swUGVuV3ovL1Z0NXZIY0h6Q0hqUjJiUzE4L2duRG5xMWlMeXp3?=
 =?utf-8?B?RnBFWnplTnJKT1NEWnVQUXlDWlQ2WVBoc3hreHBZK1NSSWk0bEJZQjdFVFpR?=
 =?utf-8?B?bkV3aGtlekF3VEhCMi9WdVF6Z1lTb0x4U1RaQVk0bXlLai9mWEJ4MEpobkNY?=
 =?utf-8?B?K1RlWktBK1BTWXFxT05tcjdLZ29sUkVpaXB4bExQQ3lDeS9lQ3Y5ZVBpZVRB?=
 =?utf-8?B?ZjMwUWY1RURyd0RuRDI2Z3dEMEN5emNsU3lwT0dpWmRIYUVyK1dDeExnNFVZ?=
 =?utf-8?B?OXpFbHRFcUlHWnoxM2F0emp1ckdoQmxpaTdNcnhMYnBnV2FlM1l5NGM4dGRs?=
 =?utf-8?B?R3VJanNhQ09ucGlPU2VaaGF3MUlKdjR2VjVVSEZ6ajMwMVExdndHZ0dVVlFH?=
 =?utf-8?B?Qkd6QjhWMStqMEE4NDhEQnNDQjVFOUF5SlBBZ3d6Wmtqa3pnRUxqUC9yRm9v?=
 =?utf-8?Q?khyCYCWQXaLnHqfUf8uoQsfIy?=
X-MS-Exchange-CrossTenant-Network-Message-Id: edc89cb9-b946-4182-53b2-08dbfbfb42bd
X-MS-Exchange-CrossTenant-AuthSource: PH7PR11MB5984.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Dec 2023 16:47:57.9449
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: m6L1nYZ3BrQaOliukXDnpB2KSyqsJsixLaptn379ZAhbkZXDkvOi6qf4L92469PqwoGhVuOiWm1iZEOxU8kMxQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB7348
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,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 howler.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 (howler.vger.email [0.0.0.0]); Wed, 13 Dec 2023 08:48:40 -0800 (PST)


On 12/12/23 20:12, dinghao.liu@zju.edu.cn wrote:
>>
>> On 12/10/23 03:27, Dinghao Liu wrote:
>>> Use the scope based resource management (defined in
>>> linux/cleanup.h) to automate resource lifetime
>>> control on struct btt_sb *super in discover_arenas().
>>>
>>> Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
>>> ---
>>>  drivers/nvdimm/btt.c | 12 ++++--------
>>>  1 file changed, 4 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
>>> index d5593b0dc700..ff42778b51de 100644
>>> --- a/drivers/nvdimm/btt.c
>>> +++ b/drivers/nvdimm/btt.c
>>> @@ -16,6 +16,7 @@
>>>  #include <linux/fs.h>
>>>  #include <linux/nd.h>
>>>  #include <linux/backing-dev.h>
>>> +#include <linux/cleanup.h>
>>>  #include "btt.h"
>>>  #include "nd.h"
>>>  
>>> @@ -847,7 +848,7 @@ static int discover_arenas(struct btt *btt)
>>>  {
>>>  	int ret = 0;
>>>  	struct arena_info *arena;
>>> -	struct btt_sb *super;
>>> +	struct btt_sb *super __free(kfree) = NULL;
>>>  	size_t remaining = btt->rawsize;
>>>  	u64 cur_nlba = 0;
>>>  	size_t cur_off = 0;
>>> @@ -860,10 +861,8 @@ static int discover_arenas(struct btt *btt)
>>>  	while (remaining) {
>>>  		/* Alloc memory for arena */
>>>  		arena = alloc_arena(btt, 0, 0, 0);
>>> -		if (!arena) {
>>> -			ret = -ENOMEM;
>>> -			goto out_super;
>>> -		}
>>> +		if (!arena)
>>> +			return -ENOMEM;
>>>  
>>>  		arena->infooff = cur_off;
>>>  		ret = btt_info_read(arena, super);
>>> @@ -919,14 +918,11 @@ static int discover_arenas(struct btt *btt)
>>>  	btt->nlba = cur_nlba;
>>>  	btt->init_state = INIT_READY;
>>>  
>>> -	kfree(super);
>>>  	return ret;
>>>  
>>>   out:
>>>  	kfree(arena);
>>>  	free_arenas(btt);
>>> - out_super:
>>> -	kfree(super);
>>>  	return ret;
>>>  }
>>>  
>>
>> I would do the allocation like something below for the first chunk. Otherwise the rest LGTM. 
>>
>> diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
>> index d5593b0dc700..143921e7f26c 100644
>> --- a/drivers/nvdimm/btt.c
>> +++ b/drivers/nvdimm/btt.c
>> @@ -16,6 +16,7 @@
>>  #include <linux/fs.h>
>>  #include <linux/nd.h>
>>  #include <linux/backing-dev.h>
>> +#include <linux/cleanup.h>
>>  #include "btt.h"
>>  #include "nd.h"
>>  
>> @@ -845,25 +846,23 @@ static void parse_arena_meta(struct arena_info *arena, struct btt_sb *super,
>>  
>>  static int discover_arenas(struct btt *btt)
>>  {
>> +       struct btt_sb *super __free(kfree) =
>> +               kzalloc(sizeof(*super), GFP_KERNEL);
>>         int ret = 0;
>>         struct arena_info *arena;
>> -       struct btt_sb *super;
>>         size_t remaining = btt->rawsize;
>>         u64 cur_nlba = 0;
>>         size_t cur_off = 0;
>>         int num_arenas = 0;
>>  
>> -       super = kzalloc(sizeof(*super), GFP_KERNEL);
>>         if (!super)
>>                 return -ENOMEM;
>>  
>>         while (remaining) {
>>                 /* Alloc memory for arena */
> 
> It's a little strange that we do not check super immediately after allocation.
> How about this:
> 
>  static int discover_arenas(struct btt *btt)
>  {
>         int ret = 0;
>         struct arena_info *arena;
> -       struct btt_sb *super;
>         size_t remaining = btt->rawsize;
>         u64 cur_nlba = 0;
>         size_t cur_off = 0;
>         int num_arenas = 0;
>  
> -       super = kzalloc(sizeof(*super), GFP_KERNEL);
> +       struct btt_sb *super __free(kfree) = 
> +               kzalloc(sizeof(*super), GFP_KERNEL);
>         if (!super)
>                 return -ENOMEM;
>  
>         while (remaining) {
>  

That's fine by me