On Thu, Apr 1, 2021 at 3:59 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> On Thu, Apr 1, 2021 at 1:20 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > On Wed, Mar 31, 2021 at 5:12 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> > > On Wed, Mar 31, 2021 at 4:04 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > > > On Tue, Mar 30, 2021 at 9:16 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> >
> > ...
> >
> > > > > -int avtab_duplicate(struct avtab *new, struct avtab *orig)
> > > > > +int avtab_alloc(struct avtab *h, u32 nrules)
> > > > > {
> > > > > - int i;
> > > > > - struct avtab_node *node, *tmp, *tail;
> > > > > -
> > > > > - memset(new, 0, sizeof(*new));
> > > > > + int rc;
> > > > > + u32 shift = 0;
> > > > > + u32 work = nrules;
> > > > > + u32 nslot = 0;
> > > > >
> > > > > - new->htable = kvcalloc(orig->nslot, sizeof(void *), GFP_KERNEL);
> > > > > - if (!new->htable)
> > > > > - return -ENOMEM;
> > > > > - new->nslot = orig->nslot;
> > > > > - new->mask = orig->mask;
> > > > > -
> > > > > - for (i = 0; i < orig->nslot; i++) {
> > > > > - tail = NULL;
> > > > > - for (node = orig->htable[i]; node; node = node->next) {
> > > > > - tmp = kmem_cache_zalloc(avtab_node_cachep, GFP_KERNEL);
> > > > > - if (!tmp)
> > > > > - goto error;
> > > > > - tmp->key = node->key;
> > > > > - if (tmp->key.specified & AVTAB_XPERMS) {
> > > > > - tmp->datum.u.xperms =
> > > > > - kmem_cache_zalloc(avtab_xperms_cachep,
> > > > > - GFP_KERNEL);
> > > > > - if (!tmp->datum.u.xperms) {
> > > > > - kmem_cache_free(avtab_node_cachep, tmp);
> > > > > - goto error;
> > > > > - }
> > > > > - tmp->datum.u.xperms = node->datum.u.xperms;
> > > > > - } else
> > > > > - tmp->datum.u.data = node->datum.u.data;
> > > > > -
> > > > > - if (tail)
> > > > > - tail->next = tmp;
> > > > > - else
> > > > > - new->htable[i] = tmp;
> > > > > -
> > > > > - tail = tmp;
> > > > > - new->nel++;
> > > > > + if (nrules != 0) {
> > > > > + while (work) {
> > > > > + work = work >> 1;
> > > >
> > > > Extra horizontal spaces are awkward and bad.
> > > >
> > > > > + shift++;
> > > > > }
> > > > > + if (shift > 2)
> > > > > + shift = shift - 2;
> > > >
> > > > Since we are getting nit-picky with this code, why not make the
> > > > loop/if a bit more elegant? How about something like below?
> > > >
> > > > u32 shift = 2;
> > > > u32 work = nrules >> 4;
> > > > while (work) {
> > > > work >>= 1;
> > > > shift++;
> > > > }
> > >
> > > I think you meant:
> > > u32 shift = 0;
> > > u32 work = nrules >> 2;
> > > while (work) {
> > > work >>= 1;
> > > shift++;
> > > }
> > >
> > > ...which is equivalent to the current code and yes, I'll use that :)
> >
> > Well, no, not really, but that's okay as looking at it now we both got
> > it wrong :)
> >
> > Basically I wanted to avoid the odd problem where the current code has
> > a dip in the number of slots/buckets when nrules is equal to 4, 5, 6,
> > or 7. While the code I proposed yesterday did that, it inflated the
> > number of buckets beyond the current code; your suggestion had
> > problems with low numbers resulting in zero buckets.
>
> Aah, I wrongly parsed the "if (shift > 2) shift -= 2;" statement...
> Yeah, I guess even the original intent was different than what the
> code does.
I think we all mis-interpreted that bit of code at some point. I
seriously doubt that dip was what the original author intended :)
> Anyway, my code doesn't result in zero buckets, at worst in 1 bucket
> (nslot = 1 << shift) ...
Sorry, yes, I mis-spoke (mis-typed?); I was talking about the
shift/exponent value.
> So I'll argue that my proposed solution is actually slightly better
> (and avoids adding a new magic number).
The magic number argument isn't really valid as both approaches use
them to some degree. Creating a #define constant is overkill here,
but I guess a short comment wouldn't be a bad idea if you wanted to
add one; I'm not going to require it in this case.
Since we are at -rc5 I really want to wrap this up soon so I'm going
to make one final suggestion:
shift = 1;
work = nrules >> 3;
while (work) {
work >>= 1;
shift++;
}
... this preserves the original nslot count, minus the bump/dip seen
with low rule numbers. The shift value starts at 1, increases to 2
when the rules reach 8, increases to 3 when the rules reach 16, and so
on. Of the approaches we've discussed, I believe it is the most
faithful to original intent.
--
paul moore
www.paul-moore.com
