On Tue, Mar 07, 2023 at 03:27:17PM -0800, Axel Rasmussen wrote:
+#define MFILL_ATOMIC_MODE_BITS (const_ilog2(NR_MFILL_ATOMIC_MODES - 1) + 1)
Here IIUC it should be "const_ilog2(NR_MFILL_ATOMIC_MODES) + 1", but maybe.. we don't bother and define every bit explicitly?
If my reading of const_ilog2's definition is correct, then:
const_ilog2(4) = 2 const_ilog2(3) = 1 const_ilog2(2) = 1
For either 3 or 4 modes, we need 2 bits to represent them (0, 1, 2, 3), i.e. we want MFILL_ATOMIC_MODE_BITS = 2. I think this is correct as is, because const_ilog2(4 - 1) + 1 = 2, and const_ilog2(3 - 1) + 1 = 2.
In other words, I think const_ilog2 is defined as floor(log2()), whereas what we want is ceil(log2()).
You're right.
The benefit of doing this vs. just doing defines with fixed values is, if we ever added a new mode, we wouldn't have to do bit twiddling and update the mask, flag bits, etc. - it would happen "automatically". I prefer it this way, but I agree it is a matter of opinion / taste. :) If you or others feel strongly this is overcomplicated, I can take the other approach.
I don't know what this will look like at last. The thing is if you plan to define MFILL_ATOMIC_* with __bitwise I think it'll stop working with any calculations upon it.
I don't worry on growing modes, as I don't expect it to happen a lot.
No strong opinion here, as long as sparse won't complain.
Thanks,