Re: [PATCH] mm: cma: Retry from the beginning of cma area if all blocks are busy
Am 06.01.17 um 18:30 schrieb valdis.kletni...@vt.edu:
> On Fri, 06 Jan 2017 18:12:41 +0100, Johannes Thoma said:
>
>> Am 06.01.17 um 17:54 schrieb valdis.kletni...@vt.edu:
>>> On Fri, 06 Jan 2017 17:16:11 +0100, johan...@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
___
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: [PATCH] mm: cma: Retry from the beginning of cma area if all blocks are busy
Hi Vladis,
Thanks for your quick reply.
Am 06.01.17 um 17:54 schrieb valdis.kletni...@vt.edu:
> On Fri, 06 Jan 2017 17:16:11 +0100, johan...@johannesthoma.com said:
>> From: Johannes Thoma
>
>> 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@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
