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

[Johannes Thoma]

Am 06.01.17 um 18:30 schrieb Valdis.Kletnieks at vt.edu:
> On Fri, 06 Jan 2017 18:12:41 +0100, Johannes Thoma said:
>
>> 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:
>
>>>> +			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:
>
> No, you missed my point.
>
> We're inside a mutex_lock() region.  We hit the goto, and drive
> bitmap_find_next_zero_are_off() again.  How do we know that we're not
> just going to spin our wheels 10 times and fall out?  What will change
> the bitmap_find_next_zero_area_offset() result?
>
Correct me if I am wrong but setting start from a high value where
no more memory is free back to zero should change the result. The
scenario I had is:

start is 0 -> bitmap_find_next_zero_off succeeds
alloc_contig_range fails with -EBUSY
start is incremented by the size of the requested block
start is near the end of the area, hence bitmap_find_next_zero_off fails
(new) now we set start again back to 0, so
	bitmap_find_next_zero_off succeeds again (it might also fail
	if someone else just allocated memory in which case we have
	to (and do) fail).
alloc_contig_range succeeds.

> Remember - there's a mutex_lock() around that call for a *reason*.  And
> I suspect the reason is specifically so nobody else *can* monkey with
> the cma area.
>
> How is this better than just dropping the mutex, doing an mdelay(), and
> then re-taking it?
>
That would be probably the best way to do it. I suspect you mean
something like:

                         if (start != 0) {
                                 retries--;
                                 if (retries > 0) {
                                         mutex_unlock(&cma->lock);
                                         mdelay(1);
                                         start = 0;
                                         mutex_lock(&cma->lock);
                                         goto scan_again;
                                 }
                         }

Hmm..it seems that that would require more work. Thank you for your
input,

- Johannes