[PATCH] mm: cma: Retry from the beginning of cma area if all blocks are busy

[Johannes Thoma]

Hi Vladis,

Thanks for your quick reply.

Am 06.01.17 um 17:54 schrieb Valdis.Kletnieks at vt.edu:
> On Fri, 06 Jan 2017 17:16:11 +0100, johannes at johannesthoma.com said:
>> From: Johannes Thoma <johannes at johannesthoma.com>
>
>> 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