Hi Vladis,
Thanks for your quick reply.
Am 06.01.17 um 17:54 schrieb Valdis.Kletnieks@xxxxxx:
> On Fri, 06 Jan 2017 17:16:11 +0100, johannes@xxxxxxxxxxxxxxxxx said:
>> From: Johannes Thoma <johannes@xxxxxxxxxxxxxxxxx>
>
>> This patch introduces a little extra loop that causes cma_alloc to
>> rescan from the beginning when all checked memory areas are busy.
>
>> for (;;) {
>> mutex_lock(&cma->lock);
>> +scan_again:
>> bitmap_no = bitmap_find_next_zero_area_off(cma->bitmap,
>> bitmap_maxno, start, bitmap_count, mask,
>> offset);
>> if (bitmap_no >= bitmap_maxno) {
>
> It worries me that a few lines above, we have
>
> if (bitmap_count > bitmap_maxno)
> return NULL;
>
> In this case, is >= correct rather than > ?
>
That might be the case, but would be an extra patch.
>> + * Restart from the beginning if all areas were busy.
>> + * This handles false failures when count is close
>> + * to overall CMA size and the checked areas were
>> + * busy temporarily.
>> + */
>> + if (start != 0 && retries--) {
>
> Do we have any guarantee, or even good reason to believe, that this
> will eventually make forward progress, or can the goto hang everything?
> I'm not thrilled by a potentially unbounded loop inside a mutex_lock(),
> especially when you holding the mutex may be preventing something else
> from changing things so forward progress can be made....
>
Good point. That is what the retries variable is good for. To make this
more obvious, I should write:
if (start != 0) {
retries--;
if (retries > 0) {
start = 0;
goto scan_again;
}
}
(with
int retries = 10;
at the beginning of the function).
Agreed
if (start != 0 && retries--) {
isn't good coding style.
That should restrict it to a maximum of 10 iterations. This could be
lowered if the size requested is smaller.
Best,
- Johannes
_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@xxxxxxxxxxxxxxxxx
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
