Re: [PATCH v8 RFC 1/3] sparc: Break up monolithic iommu table/lock into finer graularity pools and lock

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Sat, 2015-04-04 at 07:27 -0400, Sowmini Varadhan wrote:
> One last question before I spin out v9.. the dma_mask code
> is a bit confusing to me, so I want to make sure... the code is 
> 
> > if (limit + tbl->it_offset > mask) {
> >               limit = mask - tbl->it_offset + 1;
> >               /* If we're constrained on address range, first try
> >                * at the masked hint to avoid O(n) search complexity,
> >                * but on second pass, start at 0 in pool 0.
> >                */
> >               if ((start & mask) >= limit || pass > 0) {
> >                       spin_unlock(&(pool->lock));
> >                       pool = &(tbl->pools[0]);
> >                       spin_lock(&(pool->lock));
> >                       start = pool->start;
> 
> So here I would need to mark need_flush to true, right?

Well, no but ...

So you aren't changing any pool hint, so you don't have to mark any pool
for flushing. The "n < hint" check will do the flush later on if needed
(the one I told you to add).

You only need to force a flush if you modify the hint.

*However*, you do an unlock, which means that if you have modified any
*other* pool's hint (ie, need_flush is true), you need to actually
perform a flush.

I think that business with flushing on unlock is becoming too ugly, I
would suggest you simplify things by moving need_flush to be part of the
pool structure (maybe a flag).

IE, when you modify a pool's hint, your mark it for flushing. 

When you allocate from a pool, you flush it at the end, just before you
return success, if either it was marked for flushing or n < hint (and
clear the mark of course).

That should cleanly factor all cases in a simpler & more readable code
path. The flush is done only when needed (an actual allocation happen on
a pool), you just "mark them dirty" when you change their hints, you
don't have to worry about the lock dropped case & pool hop'ing anymore.

Basically the logic looks like that:

	.../...

	n = iommu_area_alloc(...)
	if (n == -1) {
		if (pass == 0) {
			change pool hint
			pool->need_flush = true;
			pass++;
			goto again;
		} else if (!largealloc && .... ) {
			unlock/pool hop/lock
			// no need to flush anything here
			pool->hint = pool->start;
			pool->need_flush = true;
			etc...
		} else {
			n = DMA_ERROR_CODE;
			goto fail;
		}
	}
	end = n + pages;

	if (n < pool->hint || pool->need_flush) {
		pool->need_flush = false;
		iommu->lazy_flush(...);
	}
	pool->hint = end;

	.../...

Now this can be further simplified imho. For example that logic:

+       if (pass == 0 && handle && *handle &&
+           (*handle >= pool->start) && (*handle < pool->end))
+               start = *handle;
+       else
+               start = pool->hint;

Can move above the again: label. that way, you remove the pass test
and you no longer have to adjust pool->hint in the "if (likely(pass ==
0))" failure case. Just adjust "start" and try again, which means you
also no longer need to set need_flush.

Additionally, when pool hop'ing, now you only need to adjust "start" as
well. By not resetting it, you also remove the need for marking it "need
flush". I would add to that, why not set "start" to the new pool's hint
instead of the new pool's start ? That will save some flushes, no ?

Anton, do you remember why you reset the hint when changing pool ? I
don't see that gaining us anything and it does make lazy flushing more
complex.

At that point, you have removed all setters of need_flush, that logic
can be entirely removed, or am I just too tired (it's past midnight) and
missing something entirely here ?

We only need the if (n < pool->hint) check to do the lazy flush at the
end, that's it.

Cheers,
Ben.