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

Jiri Pirko jiri at resnulli.us
Thu May 10 05:15:38 AEST 2018


Wed, May 09, 2018 at 03:56:45PM CEST, talb at mellanox.com wrote:
>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.

Np.


>
>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.

It's not needed, at all. Also, it is odd that you mention "unless noted
otherwise" since you don't ever "noted otherwise". Just remove it.


>> 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.

It really does not make sense to have it. Driver just selects it.
Without the driver, if user enables the lib himself, it has no meaning
as it is never used.


>
>
>> +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

The benefit is simple. Reader knows right away what is going on. It the
prefix is not there, the reader might think that it is something "from
the outside". Prefix provides clear context. Please maintain it.


>> 
>> > +{
>> > +	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 ?

prune_items_comparable()?


>> 
>> > +				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.

Odd.


>
>> 
>> > +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

Void *?
Why not just 0/-ESOMETHING like any other normal function? :)


>> 
>> > +		 */
>> > +		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.

ok.



>> 
>> 
>> > +
>> > + * 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.

Kzalloc() zeroes the memory. That's why.



>
>> > +
>> > +/**
>> > + * 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).

Whatever. Please note that kernel lib != userspace lib. Kernel lib is
just another kernel code. The api can be changed any time. User should
always look at the code in the same way as if he is calling any other
kernel function.


>> 
>_______________________________________________
>Linux-mlxsw mailing list
>Linux-mlxsw at lists.ozlabs.org
>https://lists.ozlabs.org/listinfo/linux-mlxsw


More information about the Linux-mlxsw mailing list