Hi Vladis, Thanks for your quick reply. Am 06.01.17 um 17:54 schrieb Valdis.Kletnieks@vt.edu:

On Fri, 06 Jan 2017 17:16:11 +0100, johannes@johannesthoma.com said:

...

From: Johannes Thoma <johannes@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