[RFC net-next mlxsw v4] lib: Introduce manager for priority base pruning lookups between tables

[Tal Bar]

Thanks for the review, I was ill so it took time to go over all, i would 
like to have few clarifications from you side.

On 03-May-18 4:58 PM, Jiri Pirko wrote:
>
>> +
>> +/**
>> + * This structure defines the management hooks for prune object.
>> + * The following hooks can be defined; unless noted otherwise, they are
>> + * optional and can be filled with a null pointer.
> The last note is pointless. You have 2 cbs, both can be null. Also, this
> is really something that is not needed to say, at all.
  I have took this phrase from netdevice.h can be deleted but why ? I 
can see only benefit here.
> diff --git a/lib/Kconfig b/lib/Kconfig
>> index e960894..16c3ce5 100644
>> --- a/lib/Kconfig
>> +++ b/lib/Kconfig
>> @@ -583,6 +583,12 @@ config PARMAN
>> config PRIME_NUMBERS
>> 	tristate
>>
>> +config PRUNE
>> +	tristate "prune"
>> +        help
>> +          This option enables the use of prune mananger for priority base
> s/mananger/manager/
> also, isn't it rather a library?
>
> Why do you need "help" here? This config option should be selected by
> drivers who need it. Please see "parman" for inspiration.
I think it's more clear like that, can be removed please suggest.
'prune' is not the only library which has help.


> +struct intersec_table_item_key {
>> +	unsigned long *mask;
>> +};
>> +
>> +struct intersec_table_item_entry {
>> +	struct rhash_head ht_node; /* Member of tablset HT */
>> +	struct intersec_table_item_key ht_key;
>> +	unsigned int mask_len;
>> +	struct list_head intersec_table_items;
>> +	struct list_head neighbor_table_items; /* can be enhanced to rbtree */
>> +};
>> +
>> +struct table;
> What's "table"? Please, maintain prefixes, for all structs, funcs,
> defines. I'm pretty sure I wrote that to you already.

For table I do agree since it's going to be exposed in the .h in v5, but 
I would like to have more general clarification

I understand the motivation for that, but I disagree over here for the 
general cases.
For .h file it is clear since it's exposed, but what is the benefit for 
.c file ?

It make the struct name too long IMO, without benefit.
For example:  struct intersec_table_item_entry --> struct 
prune_intersec_table_item_entry
>
>> +{
>> +	table_prune_list_deinit(&entry->table_prune_list);
>> +	kfree(entry);
>> +}
>> +
>> +static bool is_items_comparable(struct table *table, unsigned long *mask_a,
> "Is items comparable" ?
I checked few places in our driver that we use is_ prefix, can you 
please suggest ?
>
>> +				unsigned long *mask_b, unsigned int mask_len)
>> +{
>> +	if (table->ops->prune_mask_comparable) {
>> +		return table->ops->prune_mask_comparable(mask_a,
>> +						    mask_b,
>> +						    mask_len);
> Aligh properly please. This applied to the whole code.
>
>
>> +	}
> I'm pretty sure you didn't run checkpatch.pl. These {} should not be
> here.
>
I did run, checkpatch doesn't catch a if clause with contains multi line 
function and args.

>
>> +static bool prune_vector_set(struct table *table,
>> +			     struct table *neigh_table,
>> +			     const struct table_item_entry *table_item,
>> +			     struct neighbor_table_items *neigh_list_item,
>> +			     struct intersec_table_items *intsc_item,
>> +			     bool neigh_items)
> It is always scarry when functions like this return "bool". What does
> "true" or "false" really mean. Wouldn't it be possible to return
> 0/-ESOMETHING?
The bool signify if a item was updated or not, I can pass it has a 
variable and make this func void*,
please suggest
>
>> +		 */
>> +		list_for_each(item_pos, &entry->intersec_table_items) {
>> +			intsc_item = list_entry(item_pos,
>> +						typeof(*intsc_item),
>> +						list);
>> +
>> +			if (prune_vector_set(table,
>> +					     intersec_table->neigh_table,
>> +					     t_item,
>> +					     NULL,
>> +					     intsc_item,
>> +					     neigh_items)) {
>> +				item_entry_to_item_notify(&item_notify,
>> +							  intsc_item);
>> +				table->ops->prune_item_notify(table,
>> +							      &item_notify,
>> +							      1);
>> +				/* TODO: need to enhance to one notification
>> +				 * and remove this code block
>> +				 */
> ??
Since it's a RFC and I plan to add notification aggregation I have added 
this comment. I can remove it for now, please suggest.
>
>
>> +
>> + * prune_init - initializes a prune object which manage the pruning vector
>> + *		between tables
>> + * @mask_len:		Sets the mask len (nbits) for all tables in the prune
>> + *			object.
>> + * @ops:		caller-specific callbacks
>> + * @priv:		pointer to a private user data.
>> + *
>> + * Returns a pointer to newly created prune instance in case of success,
>> + * otherwise it returns NULL.
>> + *
>> + * Note: all locking must be provided by the caller.
>> + *
>> + * Before caller could add a table with certain mask, he has to
>> + * initialize the number of tables the prune supports.
>> + */
>> +struct prune *prune_init(unsigned int mask_len, const struct prune_ops *ops,
>> +			 void *priv)
>> +{
>> +	struct prune *prune;
>> +
>> +	if (!ops)
>> +		return NULL;
> Please use ERR_PTR macros
>
>
>> +
>> +	prune = kzalloc(sizeof(*prune), GFP_KERNEL);
>> +	if (!prune)
>> +		return NULL;
>> +
>> +	prune->table_cnt = 0;
> Pointless.

Why is it pointless ? I am using this counter later on.

>> +
>> +/**
>> + * prune_table_add - adds new table to the prune object with specific mask
>> + * @prune:	prune object
>> + * table_attr:	attribute for this table
>> + * @ret_table:	returns the table id on success otherwise sets valid as false
>> + *
>> + * Returns ENOMEM in case allocation failed.
>> + * Returns EPERM for invalid parameter.
>> + * otherwise it returns 0.
>> + *
>> + * Note: all locking must be provided by the caller.
>> + * This function may sleep so you must not call it from interrupt
>> + * context or with spin locks held.
> You don't need to say that. User should look and see.
>
There is probably misconception here from my side. Usually a library 
user doesn't want to read all the code in the lib, this
is why user would like to read the API documentation (there are actually 
engineers which explains lib and their usage in LWN).
>