Message-ID: <a6dd44e2-6ea6-d085-0131-1e9bac49461a@oracle.com>
Date:   Fri, 3 Dec 2021 16:04:11 -0500
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.3.2
Subject: Re: [PATCH] xen-blkfront: Use the bitmap API when applicable
Content-Language: en-US
To:     Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
        Joe Perches <joe@perches.com>, Juergen Gross <jgross@suse.com>,
        sstabellini@kernel.org, roger.pau@citrix.com, axboe@kernel.dk
Cc:     xen-devel@lists.xenproject.org, linux-block@vger.kernel.org,
        linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org
References: <1c73cf8eaff02ea19439ec676c063e592d273cfe.1638392965.git.christophe.jaillet@wanadoo.fr>
 <c529a221-f444-ad26-11ff-f693401c9429@suse.com>
 <d8f87c17-75d1-2e6b-65e1-23adc75bb515@wanadoo.fr>
 <6fcddba84070c021eb92aa9a5ff15fb2a47e9acb.camel@perches.com>
 <3d71577f-dabe-6e1a-4b03-2a44f304b702@wanadoo.fr>
 <863f2cddacac590d581cda09d548ee0a652df8a1.camel@perches.com>
 <1e9291c6-48bb-88e5-37dc-f604cfa4c4db@wanadoo.fr>
From:   Boris Ostrovsky <boris.ostrovsky@oracle.com>
In-Reply-To: <1e9291c6-48bb-88e5-37dc-f604cfa4c4db@wanadoo.fr>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?c3VySmtaRHpGQ0d3a3h5U0Nqck4rVzJ4TGJCUzg4STAzeGwreFdGOGV6QzA1?=
 =?utf-8?B?bUhMMUhBUlg5OXFaRU91cDJxb2xZR0RxT3NubTVVM1VDNkJaVzlGaXFXanlU?=
 =?utf-8?B?c2NtZlNIejQwY2dQNy9reStmUFBvQ0l5MmpRNFFCN0JmK0MzQlAzK3NRcDBK?=
 =?utf-8?B?WENKY0ZDVjRheWl6dklOdmhjaHltTDhzN2Nqa0pCeFRqUVZraDRyUDNOS0Rw?=
 =?utf-8?B?NEN4dll1NXoycWFxRFd1emR3S2RVNjRGZDhEdWVrRG4xYXNscFozd2ZJTDJv?=
 =?utf-8?B?SVM3d0tnRTJ4RkUvTVBWY1BCaHNNS1NUeWpZS2xYOTlodVBETythYzlCdEp0?=
 =?utf-8?B?SDFTMDQvekpkWWI3UGp2RDlSTDhmL2xQK1FYZG1GVmJENFMzN2oydmdiT053?=
 =?utf-8?B?b3FJUlBMdkM0djJ3dkdFTEVZekhMNG9tTFVJaGZzQWhweXRnRStLbGhLS0lT?=
 =?utf-8?B?SFdJSXZXMFpIQkp5NldCZUo0Zjk4eTlVUDBHL3BacUpBY2x4blNsclV3Qy8x?=
 =?utf-8?B?djlQTDNtb2dBWDVWRG9FY2dta1ZMK0NsQkdXS05saFlSY0lYV2JJdzgwclhD?=
 =?utf-8?B?cS9yeFBqc0wvNDZKRUFKSGVEN3FBeEhXNWZ6Ukg4MkZNYTRTeEErckhYT0k2?=
 =?utf-8?B?YWgwRm15U3FlV0FnT1dPRmNFZUN4clVrMURDNUIydEovaGxoVGJZVzZ4SjU3?=
 =?utf-8?B?TXYyMDRKeEtqUmFxR0tlRkJYNjAzR0xQZFUzWStoaFROb2FUeHFtU2VoeHRw?=
 =?utf-8?B?a1M1c1FQb2hJb2c1aTdpTUg3TlVJSmRnZ09tREF4TklhZUVBc1F1MWF6Z3Jn?=
 =?utf-8?B?NUQvQ1ozeThVQUcwVEQxc3dNZEppVEJOWXVyU1RrRDRYNjVSUWNvSmJ5eEJ1?=
 =?utf-8?B?R0d0VFVVVDd5YlErMll3TmYwbUtheC9Ka2NqRVFjaEM2bDk0TWdXc2xMNTE3?=
 =?utf-8?B?VGJ3ZEhVWkhYM1E1SXhHTjIxOXA3a2dWQlRJZjQ0Qytuc0htUkxkU1VSZVBX?=
 =?utf-8?B?Z3ZFMThlUFVvZmlKbVA2a0locU4ydlBiUWtSVFJoNmZLUWpBcTRTMkVaMnV5?=
 =?utf-8?B?cUJqeFNtMmdOSWMvQ3QrdzFkZnFRWXY1UWxmYlJXemY0cDQ0eGd5b2ZnclJs?=
 =?utf-8?B?RjBjcDNFWWdnNXhUL2U2VGJOQXdIR2FENVBHY0RGbGhDbW1FV0MrZDRSYUpC?=
 =?utf-8?B?Z04wMllyQUV0c1h2Mjl6dXlYN1RJUnpCTFZHdlc2WGQ0cWtUeDVJam5OV3Zy?=
 =?utf-8?B?cllVREZxc0U0clJOaDJMNzlsdEhlOEEzQXA1c3JwUUM3elluSnBFNFJJK21H?=
 =?utf-8?B?cnRNbjBVUXRXWm9nUFpaRG5JTGFwUFIyK0dWRTI4VUVUdnhObWVHalJNbjlr?=
 =?utf-8?B?bHV3Y1UrMWZUK0xudHF3c2ljWGNIWDBJWCtneHpkZVRabWl6SHlIV1J0dFNC?=
 =?utf-8?B?QnliMGNiZzFYTVF3NSswT1ltUHJSQkJNT1BCbDk0VitUazZLQWpoNUh0djRK?=
 =?utf-8?B?VVJzZzlCRjlzeEJkR2crbitDUDIyd1oybnBiOURoc09SSkk1eVVxWWhOMWVr?=
 =?utf-8?B?RXVjbHRYZDBSN0pobnkxM3h1bW1od0UzellST2ZqbVYrVEhkd2xuVWVYQ1N4?=
 =?utf-8?B?WXpnOHh4MDNzSmhkNDhacDFkTnNsWVY1ZDN0eExPbFZ5cDJzWUVrVXI0THI1?=
 =?utf-8?B?b2V2RVdudjdxWXhZQXRqL1YweVo0YkRnejN0TE80b0xVSGVMdHIxakFCVURr?=
 =?utf-8?B?WXNBdjRZSGhVRE05KzgrNzR0NDhKVW9icUpjWlB4MFd1OStXRzlac2hWM3FT?=
 =?utf-8?B?SytaTnJFSmpTektYRlVNcmtJRW1ReDBhazFlR3pzNDJ0ZW5xWXNaNVBwYjNM?=
 =?utf-8?B?MTJvYmg1UlpIb2I3NEc3Uk9oVHVYWFFJZjlIaHJkSnBKVGtpaFZWWnVYUDht?=
 =?utf-8?B?WEhUMGY5WGNOQjhsaVQzT0YyZmV4eXRId3FCV1FvcVJ6MzRYc09Sb3FIRUp3?=
 =?utf-8?B?cWhQNWhWSDRUTndvOGNWUUFkaDVTMzFBTmF2YzJUVkltaHl6QWRPbkhGRkFL?=
 =?utf-8?B?L1pCaGFhYnVEWGZkZGRtRGhKYUZ4SkdjZ2JVd0NoazFORS9TU3NjUzUzL3Rn?=
 =?utf-8?B?bDNNL2RSa2pSUkpQV1Vac2x3Wm9kNVhlTTExbUZ2dVVoMlBmdVFwSHhxVGdB?=
 =?utf-8?B?N3hjVVNsUTg4RjY4c2kyaUN0K09wM2RENHdjWStVY05jeUVFVVA0c0IwZWRx?=
 =?utf-8?Q?MlODytgP7Ek76mPPH0cvDlypGQhrqVRRxlEP1jzUiI=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2ea824a3-aec1-47d1-7fdd-08d9b6a078c1
X-MS-Exchange-CrossTenant-AuthSource: BLAPR10MB5009.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Dec 2021 21:04:18.7919
 (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: EjCh/p+jwiH28u3dulEacD0hwO1CubbnGss8XBo/JG9q5eZHT3GTwLhc7pay1MBABOc3puHV3zee4R2StHsdoZuX+PAqN9OO69Cmo6JQ+80=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR10MB4192
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10187 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 malwarescore=0 mlxscore=0
 suspectscore=0 mlxlogscore=999 spamscore=0 phishscore=0 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000
 definitions=main-2112030133
X-Proofpoint-GUID: K1yRVTY7jyhcxZSyIV47rN2jvdC2qh4u
X-Proofpoint-ORIG-GUID: K1yRVTY7jyhcxZSyIV47rN2jvdC2qh4u
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 12/3/21 10:54 AM, Christophe JAILLET wrote:
> Le 03/12/2021 à 04:03, Joe Perches a écrit :
>> On Thu, 2021-12-02 at 20:07 +0100, Christophe JAILLET wrote:
>>> Le 02/12/2021 à 19:16, Joe Perches a écrit :
>>>> On Thu, 2021-12-02 at 19:12 +0100, Christophe JAILLET wrote:
>>>>> Le 02/12/2021 à 07:12, Juergen Gross a écrit :
>>>>>> On 01.12.21 22:10, Christophe JAILLET wrote:
>>>>>>> Use 'bitmap_zalloc()' to simplify code, improve the semantic and avoid
>>>>>>> some open-coded arithmetic in allocator arguments.
>>>>>>>
>>>>>>> Also change the corresponding 'kfree()' into 'bitmap_free()' to keep
>>>>>>> consistency.
>>>>>>>
>>>>>>> Use 'bitmap_copy()' to avoid an explicit 'memcpy()'
>>>> []
>>>>>>> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
>>>> []
>>>>>>> @@ -442,16 +442,14 @@ static int xlbd_reserve_minors(unsigned int
>>>>>>> minor, unsigned int nr)
>>>>>>>         if (end > nr_minors) {
>>>>>>>             unsigned long *bitmap, *old;
>>>>>>> -        bitmap = kcalloc(BITS_TO_LONGS(end), sizeof(*bitmap),
>>>>>>> -                 GFP_KERNEL);
>>>>>>> +        bitmap = bitmap_zalloc(end, GFP_KERNEL);
>>>>>>>             if (bitmap == NULL)
>>>>>>>                 return -ENOMEM;
>>>>>>>             spin_lock(&minor_lock);
>>>>>>>             if (end > nr_minors) {
>>>>>>>                 old = minors;
>>>>>>> -            memcpy(bitmap, minors,
>>>>>>> -                   BITS_TO_LONGS(nr_minors) * sizeof(*bitmap));
>>>>>>> +            bitmap_copy(bitmap, minors, nr_minors);
>>>>>>>                 minors = bitmap;
>>>>>>>                 nr_minors = BITS_TO_LONGS(end) * BITS_PER_LONG;
>>>>
>>>>         nr_minors = end;
>>>> ?
>>>>
>>>
>>> No,
>>> My understanding of the code is that if we lack space (end > nr_minors),
>>> we need to allocate more. In such a case, we want to keep track of what
>>> we have allocated, not what we needed.
>>> The "padding" bits in the "long align" allocation, can be used later.
>>
>>>
>>> first call
>>> ----------
>>> end = 65
>>> nr_minors = 63
>>>
>>> --> we need some space
>>> --> we allocate 2 longs = 128 bits
>>> --> we now use 65 bits of these 128 bits
>>
>> or 96, 32 or 64 bit longs remember.
>
> 32 and 64 for sure, but I was not aware of 96. On which arch?
>
>>
>>>
>>> new call
>>> --------
>>> end = 68
>>> nr_minors = 128 (from previous call)
>>
>> The initial allocation is now bitmap_zalloc which
>> specifies only bits and the nr_minors is then in
>> BITS_TO_LONGS(bits) * BITS_PER_LONG
>>
>> Perhaps that assumes too much about the internal
>> implementation of bitmap_alloc
>>
>>
>
> I get your point now, and I agree with you.
>
> Maybe something as what is done in mc-entity.c?
> Explicitly require more bits (which will be allocated anyway), instead of taking advantage (read "hoping") that it will be done.
>
> Could be:
>
> @@ -440,26 +440,25 @@ static int xlbd_reserve_minors(unsigned int minor, unsigned int nr)
>      int rc;
>
>      if (end > nr_minors) {
>          unsigned long *bitmap, *old;
>
> -        bitmap = kcalloc(BITS_TO_LONGS(end), sizeof(*bitmap),
> -                 GFP_KERNEL);
> +        end = ALIGN(end, BITS_PER_LONG);
> +        bitmap = bitmap_zalloc(end, GFP_KERNEL);
>          if (bitmap == NULL)
>              return -ENOMEM;
>
>          spin_lock(&minor_lock);
>          if (end > nr_minors) {
>              old = minors;
> -            memcpy(bitmap, minors,
> -                   BITS_TO_LONGS(nr_minors) * sizeof(*bitmap));
> +            bitmap_copy(bitmap, minors, nr_minors);
>              minors = bitmap;
> -            nr_minors = BITS_TO_LONGS(end) * BITS_PER_LONG;
> +            nr_minors = end;
>          } else
>              old = bitmap;
>          spin_unlock(&minor_lock);
> -        kfree(old);
> +        bitmap_free(old);
>      }
>
>      spin_lock(&minor_lock);
>      if (find_next_bit(minors, end, minor) >= end) {


I don't think this will work anymore, we may now fail if another thread gets a minor above the original (i.e. no aligned) @end.


-boris


> bitmap_set(minors, minor, nr);
> @@ -2608,11 +2607,11 @@ static void __exit xlblk_exit(void)
>  {
>      cancel_delayed_work_sync(&blkfront_work);
>
>      xenbus_unregister_driver(&blkfront_driver);
>      unregister_blkdev(XENVBD_MAJOR, DEV_NAME);
> -    kfree(minors);
> +    bitmap_free(minors);
>  }
>  module_exit(xlblk_exit);
>
>  MODULE_DESCRIPTION("Xen virtual block device frontend");
>  MODULE_LICENSE("GPL");
>
>