<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Thanks for the review.<br>
Most of the comments are already taken care, I have few questions <br>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">On 05-Jun-18 5:40 PM, Jiri Pirko wrote:<br>
</div>
<blockquote type="cite" cite="mid:20180605144009.GE2126@nanopsycho">
<pre class="moz-quote-pre" wrap="">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 :/</pre>
</blockquote>
<font face="Calibri">I was in RD so couldn't complete it on days...<br>
Anyhow now I have returned back to office so response should be
quick now. </font><tt> </tt><br>
<blockquote type="cite" cite="mid:20180605144009.GE2126@nanopsycho">
<pre class="moz-quote-pre" wrap="">My head is spinning a little bit... This seems quite complex that makes
one wonder if the complexicity is really necessary.
</pre>
</blockquote>
<blockquote type="cite" cite="mid:20180605144009.GE2126@nanopsycho">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">No need. Remove this section. (I have a feeling that I already asked that)</pre>
</blockquote>
<font face="Calibri">This section is Synchronization & critical
sections ? or also Database ?<br>
Before that you have asked to remove the Synchronization from the
APIs not from here. </font><br>
<blockquote type="cite" cite="mid:20180605144009.GE2126@nanopsycho"><br>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ * 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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">I have no clue what "A" and "A*" is :O</pre>
</blockquote>
<font size="+1"><tt> </tt></font>
<div class="WordSection1"><font size="+1"><tt> </tt></font>It's
just a notation for the direction of the the prune calculation <br>
</div>
<div class="WordSection1">A*-->A: calculate my own rule</div>
<div class="WordSection1">A-->A*: calculate other rules in the
prune object.<br>
</div>
<blockquote type="cite" cite="mid:20180605144009.GE2126@nanopsycho">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ */
+
+#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 {
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">You don't use this outside prune.c. Please move it there.</pre>
</blockquote>
I'm using it in test_prune.c<br>
<blockquote type="cite" cite="mid:20180605144009.GE2126@nanopsycho">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ struct prune_table *table;
+ bool is_prune; /* True if item should prune this table otherwise not */
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">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".
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ struct list_head list;
+ void *priv;
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">When I see list_head and void *priv as a part of exported header, I'm
starting to be really scarred...</pre>
</blockquote>
Added an explanation to priv, you will see it in next version<br>
it's the priv pf the table<br>
<blockquote type="cite" cite="mid:20180605144009.GE2126@nanopsycho">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+};
+
+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);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">item_cnt is set to 1 and never used. Why do you carry this? Please
remove.</pre>
</blockquote>
You are correct, it was used for notification aggregation but i have
change this interface so no longer needed. <br>
<blockquote type="cite" cite="mid:20180605144009.GE2126@nanopsycho">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ 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,
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">"_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()".
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ void *priv);
+void prune_fini(struct prune *prune);
+struct prune_table *prune_table_add(struct prune *prune,
+ struct prune_table_attr *table_attr);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Again, since the table is allocated and initialized inside, do
"_create()" and "_destroy()".
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+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);
</pre>
</blockquote>
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.
</blockquote>
I understand the motivation here, the user doesn't need to allocate
prune_item, but if anyhow in the driver we already allocate it<br>
the *mask filed for the rule, why does the library should do it
again ?<br>
Regarding struct prune_table_attr, should I remove it also ?
<blockquote type="cite" cite="mid:20180605144009.GE2126@nanopsycho">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+int prune_item_remove(struct prune *prune, struct prune_table *table,
+ struct prune_item *item);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Should not return int. Just void. Please adjust the function and
whatever is it calling. </pre>
</blockquote>
What if this function fails ? should I add WARN(...) where it fails
?
<blockquote type="cite" cite="mid:20180605144009.GE2126@nanopsycho">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+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;
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">So "item" or "vector_item". Make up your mind. Consistency, consistency,
consistency!</pre>
</blockquote>
There are two different structs one is prune_item the other one is
prune_vector_item.<br>
what is not consistent here ? would you prefer prune_vector_elememt
and vector_elem ?<br>
<blockquote type="cite" cite="mid:20180605144009.GE2126@nanopsycho">
</blockquote>
<blockquote type="cite" cite="mid:20180605144009.GE2126@nanopsycho">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+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;
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">It is really weird to take an internal list and expose it outside like
this.</pre>
</blockquote>
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 <br>
<blockquote type="cite" cite="mid:20180605144009.GE2126@nanopsycho">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+}
+
+static bool
+prune_item_add_prune_vector_calc(struct prune_table *table,
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">"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".</pre>
</blockquote>
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<br>
same goes with this function: prune_item_rem_prune_vector_calc<br>
<blockquote type="cite" cite="mid:20180605144009.GE2126@nanopsycho"><br>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ * @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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">These are pointless. Please remove.</pre>
</blockquote>
Did you meant only for @table_handle or also for the error code ?<br>
<blockquote type="cite" cite="mid:20180605144009.GE2126@nanopsycho">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ * otherwise it returns 0.
+ *
+ * Note: all locking must be provided by the caller.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">"All locking"? Sounds really odd.</pre>
</blockquote>
Actually I have took the phrasing from parman lib, should I change
it to:<br>
"It's the user responsibility to care for synchronization"
<blockquote type="cite"> </blockquote>
</body>
</html>