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, -- Peter Xu
