This is naive in the sense that vm_phys_alloc_contig() effectively does the equivalent of vm_phys_find_range(), but there's currently no means of indicating an out-of-range allocation request to the caller. That would require extra plumbing in the higher-level allocators, and I'm not sure if the added complexity would be worth the trouble given that we would probably expect most allocations to succeed before reaching this point. I therefore decided to start off with what seemed like the simplest implementation.

Can we assign to m_runs only after this test is passed, so that failing this test can just 'return', rather than going to 'done' for cleanup?

Nitpick - these lines don't need to be relocated.

Perhaps we can avoid the initial call to vm_phys_find_range with:

prev_segind = start_segind;
while ((segind = vm_phys_find_range(... prev_segind ...) != -1) {

 while () {}
prev_segind = segind + 1;

}
if (prev_segind == start_segind) {

 ret = ERANGE;
goto done;

}

It's a bit of hair-splitting, but I actually just moved the computation of m_runs after the check of vm_phys_find_range(), but of course the diff tool doesn't know that. In any case I'll probably just revert this part per your suggestion below.

Good idea, to be honest I don't know why I didn't do it this way to begin with.

style(9) doesn't require all local variables to be declared at the beginning of this function anymore. This function in particular does not follow that convention, so it's a bit strange to declare err here.

The indentation here looks wrong.

I suggest annotating this with __predict_false().

Should we first assert that the domain is currently set?

There's no need for parentheses around the function call.

The duplication of this pattern suggests to me that we might want to introduce yet another domainset iterator, this time for physical memory allocations, which takes all of the constraints variables and handles checking for impossible allocations. Then there would be no need to have a public vm_domainset_iter_ignore(). I'm not sure this is a good way to go, though, especially if we need to add new fields to vm_domainset_iter or introduce a new "contig iterator" type.

More generally I wonder if it might be sensible to put all of these parameters into a struct and pass that around instead, but that's not really in the scope of this diff.

Rather than initializing this value once at the beginning of the function, I'd initialize it to false right before the loop. It doesn't affect correctness, but there's no need to preserve phys_range_exists's value across iterations of the outer loop.

Hmm, that certainly wouldn't break anything I'm doing here, but I'm not sure that calling vm_domainset_iter_ignore() on an already-cleared (or already-ignored) domain would inherently indicate a programming error either. Sort of like calling free(NULL).

I think for now I'd prefer to keep it the way I have it. I like the orthogonality of having a generic way to skip individual iterator elements, on the off chance that we may want to use vm_domainset_iter_ignore() for something different in the future.

While the semantics for free(NULL) are sensible given how much free() is used in cleanup paths, this function is used only in quite specific situations where it IMHO makes more sense to be strict. If a specific need arises, the restriction can always be removed. But I do not insist.

Fair enough.