>> + int ret = rte_hash_lookup_data(lpm->rules_tbl, (void *) &rule_key,
>> + (void **) &rule);
>> + if (ret >= 0) {
>> + /* delete the rule */
>> + rte_hash_del_key(lpm->rules_tbl, (void *) &rule_key);
>> + lpm->used_rules--;
>> + rte_mempool_put(lpm->rules_pool, rule);
>> + }
> Rather than doing a lookup and then delete, why not just try the delete
> straight off. If you want to check for the key not being present, it can be
> detected from the output of the delete call. From rte_hash.h:
> * @return
> * - -EINVAL if the parameters are invalid.
> * - -ENOENT if the key is not found.
A deleted rule has to be returned back to the mempool.
And I don't see any delete function in the rte_hash that can
return a deleted item back to a caller.
>> +
>> + return ret;
>> }
>>
>> /*
>> - * Deletes a rule
>> + * Deletes a group of rules
> Include a comment that this bulk function will rebuild the lpm table,
> rather than doing incremental updates like the regular delete function.
ok
>> + * Convert a depth to a one byte long mask
>> + */
>> +static uint8_t __attribute__((pure))
>> +depth_to_mask_1b(uint8_t depth)
>> +{
>> + /* To calculate a mask start with a 1 on the left hand side and right
>> + * shift while populating the left hand side with 1's
>> */
>> - if ((lpm == NULL) || (ips == NULL) || (depths == NULL)) {
>> - return -EINVAL;
>> + return (signed char)0x80 >> (depth - 1);
> I'd make the comment on the function a little clearer e.g. using an
example: "4 =>> 0xF0", which should remove the need to have the second comment
> above the return statement.
> An alternative that might be a little clearer for the calculation would be:
"(uint8_t)(~(0xFF >>> depth))".
I've just copied this function from rte_lpm.c and converted it to 1byte version.
I'll add an example 4 =>> 0xF0.
>> +}
>> +
>> +/*
>> + * Find a less specific rule
>> + */
>> +static struct rte_lpm6_rule*
>> +rule_find_less_specific(struct rte_lpm6 *lpm, uint8_t *ip, uint8_t depth)
>> +{
>> + if (depth == 1)
>> + return NULL;
>> +
>> + struct rte_lpm6_rule *rule;
>> + struct rte_lpm6_rule_key rule_key;
>> + rule_key_init(&rule_key, ip, depth);
>> + uint8_t mask;
>> +
>> + while (depth > 1) {
>> + depth--;
>> +
>> + /* each iteration zero one more bit of the key */
>> + mask = depth & 7; /* depth % 8 */
>> + if (mask > 0)
>> + mask = depth_to_mask_1b(mask);
>> +
>> + rule_key.depth = depth;
>> + rule_key.ip[depth >> 3] &= mask;
>> +
> It seems strange that when you adjust the depth, you also need to mask out
> bits of the key which should be ignored. Can you make the masking part of
> the hash calculation, which would simplify the logic here a lot, and if so,
> does it affect performance much?
The first version of rule_find_less_specific() was doing exactly what you are
proposing,
masking whole ipv6 address every time. But then I just couldn't stop myself from
using this shortcut since it's a performance optimization patch.
So, yes, it could be a part of the hash calculation, but why? It's definetly not
the most difficult part of the algorithm (even without this optimizations),
so it would not make life easier :)
>> }
>> --
> Rest of the patch looks fine to me, though I can't say I've followed all
> the logic paths in full detail.
> Main concern I have about the patch is the size. Is there any way this
> patch could be split up into a few smaller ones with more gradual changes?
I could try to split it in two parts. The first part will introduce the new rule
subsystem using a hashtable instead of a flat array. And the second one will
include
the rest.
> Regards,
> /Bruce
--
Alex
