| Message ID | 1420529003-22244-4-git-send-email-ying.xue@windriver.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 01/06/15 at 03:23pm, Ying Xue wrote: > Should use future table size instead of old table size to decide > whether hash table is worth being expanded. > > Signed-off-by: Ying Xue <ying.xue@windriver.com> > Cc: Thomas Graf <tgraf@suug.ch> > --- > lib/rhashtable.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) Apologies as you were probably mislead by the bug as commented on in the previous patch. I don't think we need this. future_tbl only points to a different table until the old table entries are linked from the new table. The condition in the resize check where meant to exclude this phase so we would newer get to the deferred worker while relinking is happening. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/06/2015 05:35 PM, Thomas Graf wrote: > On 01/06/15 at 03:23pm, Ying Xue wrote: >> Should use future table size instead of old table size to decide >> whether hash table is worth being expanded. >> >> Signed-off-by: Ying Xue <ying.xue@windriver.com> >> Cc: Thomas Graf <tgraf@suug.ch> >> --- >> lib/rhashtable.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) > > Apologies as you were probably mislead by the bug as commented > on in the previous patch. > > I don't think we need this. future_tbl only points to a different > table until the old table entries are linked from the new table. > The condition in the resize check where meant to exclude this > phase so we would newer get to the deferred worker while relinking > is happening. > > Thank you for above nice explanation. Regarding my understanding, as rhashtable_expand() and rhashtable_shrink() are always under the protection of "ht->mutex", the "future_tbl" and "tbl" absolutely point to the same bucket array once rhashtable_expand() or rhashtable_shrink() returns. Therefore, if rht_deferred_worker() takes the "ht->mutex" lock, the both "future_tbl" and "tbl" should point to the same bucket array. So the change made in the patch is useless for us, right? But as you pointed in above patch, there is a bug in rhashtable_wakeup_worker(). As long as ht->tbl == ht->future_tbl, we should wake up the work. OK, I will drop the patch and fix the error in patch #2. In all, thank you for quickly reviewing the series, and I would deliver the next version soon in which your all comments would be resolved. Please wait a moment. Regards, Ying -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/06/15 at 05:56pm, Ying Xue wrote: > Thank you for above nice explanation. Regarding my understanding, as > rhashtable_expand() and rhashtable_shrink() are always under the > protection of "ht->mutex", the "future_tbl" and "tbl" absolutely point > to the same bucket array once rhashtable_expand() or rhashtable_shrink() > returns. Therefore, if rht_deferred_worker() takes the "ht->mutex" lock, > the both "future_tbl" and "tbl" should point to the same bucket array. > So the change made in the patch is useless for us, right? Correct. > But as you pointed in above patch, there is a bug in > rhashtable_wakeup_worker(). As long as ht->tbl == ht->future_tbl, we > should wake up the work. OK, I will drop the patch and fix the error in > patch #2. Awesome, thanks! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/lib/rhashtable.c b/lib/rhashtable.c index 6eda22f..f5288b1 100644 --- a/lib/rhashtable.c +++ b/lib/rhashtable.c @@ -478,13 +478,14 @@ EXPORT_SYMBOL_GPL(rhashtable_shrink); static void rht_deferred_worker(struct work_struct *work) { struct rhashtable *ht; - struct bucket_table *tbl; + struct bucket_table *tbl, *new_tbl; ht = container_of(work, struct rhashtable, run_work.work); mutex_lock(&ht->mutex); tbl = rht_dereference(ht->tbl, ht); + new_tbl = rht_dereference(ht->future_tbl, ht); - if (ht->p.grow_decision && ht->p.grow_decision(ht, tbl->size)) + if (ht->p.grow_decision && ht->p.grow_decision(ht, new_tbl->size)) rhashtable_expand(ht); else if (ht->p.shrink_decision && ht->p.shrink_decision(ht, tbl->size)) rhashtable_shrink(ht);
Should use future table size instead of old table size to decide whether hash table is worth being expanded. Signed-off-by: Ying Xue <ying.xue@windriver.com> Cc: Thomas Graf <tgraf@suug.ch> --- lib/rhashtable.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)