Messages in this thread |  | | Date | Sun, 3 Nov 2013 10:51:27 -0800 | Subject | Re: [PATCH] mm: cache largest vma | From | Linus Torvalds <> |
| |
Ugh. This patch makes me angry. It looks way too ad-hoc.
I can well imagine that our current one-entry cache is crap and could be improved, but this looks too random. Different code for the CONFIG_MMU case? Same name, but for non-MMU it's a single entry, for MMU it's an array? And the whole "largest" just looks odd. Plus why do you set LAST_USED if you also set LARGEST?
Did you try just a two- or four-entry pseudo-LRU instead, with a per-thread index for "last hit"? Or even possibly a small fixed-size hash table (say "idx = (add >> 10) & 3" or something)?
And what happens for threaded models? Maybe we'd be much better off making the cache be per-thread, and the flushing of the cache would be a sequence number that has to match (so "vma_clear_cache()" ends up just incrementing a 64-bit sequence number in the mm)?
Basically, my complaints boil down to "too random" and "too specialized", and I can see (and you already comment on) this patch being grown with even *more* ad-hoc random new cases (LAST, LARGEST, MOST_USED - what's next?). And while I don't know if we should worry about the threaded case, I do get the feeling that this ad-hoc approach is guaranteed to never work for that, which makes me feel that it's not just ad-hoc, it's also fundamentally limited.
I can see us merging this patch, but I would really like to hear that we do so because other cleaner approaches don't work well. In particular, pseudo-LRU tends to be successful (and cheap) for caches.
Linus
|  |