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

[Tal Bar]

Thanks for the review.
Most of the comments are already taken care, I have few questions

On 05-Jun-18 5:40 PM, Jiri Pirko wrote:
> I went this throught quickly, I pointed out some obvious issues I can
> spot. Please fix before I will review any further. I would appreciate
> you do it in days. Every time you send a new version, my caches are
> already flushed and I start from scratch :/
I was in RD so couldn't complete it on days...
Anyhow now I have returned back to office so response should be quick now.
> My head is spinning a little bit... This seems quite complex that makes
> one wonder if the complexicity is really necessary.
>> +
>> +Database
>> +========
>> +Each prune object has a list of tables with mask.
>> +Each table consist:
>> +	* hash table for items
>> +	* list of the corresponding intersection tables (with other tables)
>> +Each intersection table consists of:
>> +	* hash table for intersects items
>> +		* each item contain two lists:
>> +			1. items of the table owner
>> +			2. items of other tables (neighbor) in the prune object.
>> +
>> +Synchronization & critical sections
>> +===================================
>> +library doesn't take responsibility on synchronization. it's the user
>> +responsibility to synchronize.
>> +
>> +Some of the APIs might sleep, please check the API headers for more information.
> No need. Remove this section. (I have a feeling that I already asked that)
This section is Synchronization & critical sections ? or also Database ?
Before that you have asked to remove the Synchronization from the APIs 
not from here.
>
>> +	* A*->A: Insert an item 'R' to a table, calculate its own prune-vector.
>> +	* A->A*: 'R' was inserted to a table, and we have to calculate the
> I have no clue what "A" and "A*" is :O
It's just a notation for the direction of the the prune calculation
A*-->A:  calculate my own rule
A-->A*:  calculate other rules in the prune object.
>> + */
>> +
>> +#ifndef _PRUNE_H
>> +#define _PRUNE_H
>> +
>> +struct prune_table;
>> +struct prune;
>> +
>> +struct prune_item {
>> +	unsigned long priority; /* Higher number means higher priority */
>> +	unsigned long *mask;
>> +	unsigned int mask_len; /* Number of bits */
>> +	void *priv;
>> +};
>> +
>> +struct prune_vector_item {
> You don't use this outside prune.c. Please move it there.
I'm using it in test_prune.c
>> +	struct prune_table *table;
>> +	bool is_prune; /* True if item should prune this table otherwise not */
> Drop the "otherwise not".
>
> Is "is_prune" really the right name? Looks like "should_prune" is more
> accurate. If not, rather name it "is_pruned".
>
>
>> +	struct list_head list;
>> +	void *priv;
> When I see list_head and void *priv as a part of exported header, I'm
> starting to be really scarred...
Added an explanation to priv, you will see it in next version
it's the priv pf the table
>> +};
>> +
>> +struct prune_item_notify {
>> +	struct prune_item item;
>> +	/* list of prune vector item */
>> +	const struct list_head *table_prune_list;
>> +};
>> +
>> +/**
>> + * 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.
>> + *
>> + * prune_item_notify:
>> + *	Called if there is a change in a the prune vector of an item or items,
>> + *	the notification can be sent on one item or more.
>> + *
>> + * prune_mask_comparable:
>> + *	Called when adding new items to a table.
>> + */
>> +struct prune_ops {
>> +	void (*prune_item_notify)(struct prune_table *table,
>> +				  struct prune_item_notify *item,
>> +				  unsigned int item_cnt);
> item_cnt is set to 1 and never used. Why do you carry this? Please
> remove.
You are correct, it was used for notification aggregation but i have 
change this interface so no longer needed.
>> +	int (*prune_mask_comparable)(unsigned long *mask_a,
>> +				     unsigned long *mask_b,
>> +				     unsigned int len);
>> +};
>> +
>> +struct prune_table_attr {
>> +	unsigned long *mask;
>> +	unsigned int mask_len; /* Number of bits */
>> +	/* Used by the priv field in strcut prune_table */
>> +	void *priv;
>> +};
>> +
>> +struct prune *prune_init(unsigned int mask_len, const struct prune_ops *ops,
> "_init()" should be used for initialization in case you already has the
> object allocated (by "_alloc()" function). If you do 2 in 1, please use
> "_create()" and "_destroy()".
>
>
>> +			 void *priv);
>> +void prune_fini(struct prune *prune);
>> +struct prune_table *prune_table_add(struct prune *prune,
>> +				    struct prune_table_attr *table_attr);
> Again, since the table is allocated and initialized inside, do
> "_create()" and "_destroy()".
>
>
>
>> +int prune_table_remove(struct prune *prune, struct prune_table *table);
>> +int prune_item_add(struct prune *prune, struct prune_table *table,
>> +		   struct prune_item *item);
> Please always, if possible, avoid structs to be passed instead of 
> simple args. In this case, just get rid of struct prune_item and pass 
> all what you need as function args. You can then rename "struct 
> prune_item_entry" to "struct prune_item". Also, I believe that you 
> should rename this to "_create" and let the function return "struct 
> prune_item_entry/struct prune_item". Later on the caller will use the 
> returned pointer for destuction. 
I understand the motivation here, the user doesn't need to allocate 
prune_item, but if anyhow in the driver we already allocate it
the *mask filed for the rule, why does the library should do it again ?
Regarding struct prune_table_attr, should I remove it also ?
>> +int prune_item_remove(struct prune *prune, struct prune_table *table,
>> +		      struct prune_item *item);
> Should not return int. Just void. Please adjust the function and
> whatever is it calling.
What if this function fails ? should I add WARN(...) where it fails ?
>> +
>> +static bool prune_item_update(const struct list_head *table_prune_list,
>> +			      struct prune_table *corresponding_table,
>> +			      bool is_prune)
>> +{
>> +	struct prune_vector_item *vector_item;
> So "item" or "vector_item". Make up your mind. Consistency, consistency,
> consistency!
There are two different structs one is prune_item the other one is 
prune_vector_item.
what is not consistent here ? would you prefer prune_vector_elememt and 
vector_elem ?
>> +
>> +static void
>> +prune_item_entry_to_item_notify(struct prune_item_notify *item_notify,
>> +				struct prune_intersec_table_item *intersec_item)
>> +{
>> +	const struct prune_table_item_entry *entry = intersec_item->entry;
>> +
>> +	item_notify->item.priority = entry->ht_key.item->priority;
>> +	item_notify->item.mask = entry->ht_key.item->mask;
>> +	item_notify->item.mask_len = entry->ht_key.item->mask_len;
>> +	item_notify->table_prune_list = &entry->table_prune_vector;
> It is really weird to take an internal list and expose it outside like
> this.
This is the reason I have const on this filed when it is exposed to the 
user, otherwise I have to copy it. please advise
>> +}
>> +
>> +static bool
>> +prune_item_add_prune_vector_calc(struct prune_table *table,
> "prune_item_add_prune_vector_calc"? What is that.
> I think that I made myself clear that the function name should be
> always: "prefix_object_action".
The prefix here is "prune_item_add" since this func is being called from 
an add flow, while the action here is calc which is being done on the 
prune_vector
same goes with this function: prune_item_rem_prune_vector_calc
>
>> + * @table_handle:	Add the item to this table identification.
>> + * @item:		The item to add.
>> + *
>> + * Returns EPERM for invalid parameter
>> + * Returns ENOMEM in case allocation failed
>> + * Returns EEXIST if item already exists.
> These are pointless. Please remove.
Did you meant only for @table_handle or also for the error code ?
>> + * otherwise it returns 0.
>> + *
>> + * Note: all locking must be provided by the caller.
> "All locking"? Sounds really odd.
Actually I have took the phrasing from parman  lib, should I change it to:
"It's the user responsibility to care for synchronization"
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linux-mlxsw/attachments/20180611/83b054b9/attachment-0001.html>