Hi Vladis, Thanks for your quick reply.
Am 06.01.17 um 17:54 schrieb [email protected]: > On Fri, 06 Jan 2017 17:16:11 +0100, [email protected] said: >> From: Johannes Thoma <[email protected]> > >> 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 [email protected] https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
