Commit 99815f5031db36414178f45e3009fb5f0e219dd4
Committed by
David S. Miller
David S. Miller
1 parent
f0c03ee0ec
net: sched: flower: don't call synchronize_rcu() on mask creation
Current flower mask creating code assumes that temporary mask that is used when inserting new filter is stack allocated. To prevent race condition with data patch synchronize_rcu() is called every time fl_create_new_mask() replaces temporary stack allocated mask. As reported by Jiri, this increases runtime of creating 20000 flower classifiers from 4 seconds to 163 seconds. However, this design is no longer necessary since temporary mask was converted to be dynamically allocated by commit 2cddd2014782 ("net/sched: cls_flower: allocate mask dynamically in fl_change()"). Remove synchronize_rcu() calls from mask creation code. Instead, refactor fl_change() to always deallocate temporary mask with rcu grace period. Fixes: 195c234d15c9 ("net: sched: flower: handle concurrent mask insertion") Reported-by: Jiri Pirko <jiri@mellanox.com> Signed-off-by: Vlad Buslov <vladbu@mellanox.com> Tested-by: Jiri Pirko <jiri@mellanox.com> Acked-by: Jiri Pirko <jiri@mellanox.com> Signed-off-by: David S. Miller <davem@davemloft.net>
Showing 1 changed file with 18 additions and 16 deletions Side-by-side Diff
net/sched/cls_flower.c
| ... | ... | @@ -320,10 +320,13 @@ |
| 320 | 320 | return rhashtable_init(&head->ht, &mask_ht_params); |
| 321 | 321 | } |
| 322 | 322 | |
| 323 | -static void fl_mask_free(struct fl_flow_mask *mask) | |
| 323 | +static void fl_mask_free(struct fl_flow_mask *mask, bool mask_init_done) | |
| 324 | 324 | { |
| 325 | - WARN_ON(!list_empty(&mask->filters)); | |
| 326 | - rhashtable_destroy(&mask->ht); | |
| 325 | + /* temporary masks don't have their filters list and ht initialized */ | |
| 326 | + if (mask_init_done) { | |
| 327 | + WARN_ON(!list_empty(&mask->filters)); | |
| 328 | + rhashtable_destroy(&mask->ht); | |
| 329 | + } | |
| 327 | 330 | kfree(mask); |
| 328 | 331 | } |
| 329 | 332 | |
| 330 | 333 | |
| ... | ... | @@ -332,9 +335,17 @@ |
| 332 | 335 | struct fl_flow_mask *mask = container_of(to_rcu_work(work), |
| 333 | 336 | struct fl_flow_mask, rwork); |
| 334 | 337 | |
| 335 | - fl_mask_free(mask); | |
| 338 | + fl_mask_free(mask, true); | |
| 336 | 339 | } |
| 337 | 340 | |
| 341 | +static void fl_uninit_mask_free_work(struct work_struct *work) | |
| 342 | +{ | |
| 343 | + struct fl_flow_mask *mask = container_of(to_rcu_work(work), | |
| 344 | + struct fl_flow_mask, rwork); | |
| 345 | + | |
| 346 | + fl_mask_free(mask, false); | |
| 347 | +} | |
| 348 | + | |
| 338 | 349 | static bool fl_mask_put(struct cls_fl_head *head, struct fl_flow_mask *mask) |
| 339 | 350 | { |
| 340 | 351 | if (!refcount_dec_and_test(&mask->refcnt)) |
| ... | ... | @@ -1346,9 +1357,6 @@ |
| 1346 | 1357 | if (err) |
| 1347 | 1358 | goto errout_destroy; |
| 1348 | 1359 | |
| 1349 | - /* Wait until any potential concurrent users of mask are finished */ | |
| 1350 | - synchronize_rcu(); | |
| 1351 | - | |
| 1352 | 1360 | spin_lock(&head->masks_lock); |
| 1353 | 1361 | list_add_tail_rcu(&newmask->list, &head->masks); |
| 1354 | 1362 | spin_unlock(&head->masks_lock); |
| ... | ... | @@ -1375,11 +1383,7 @@ |
| 1375 | 1383 | |
| 1376 | 1384 | /* Insert mask as temporary node to prevent concurrent creation of mask |
| 1377 | 1385 | * with same key. Any concurrent lookups with same key will return |
| 1378 | - * -EAGAIN because mask's refcnt is zero. It is safe to insert | |
| 1379 | - * stack-allocated 'mask' to masks hash table because we call | |
| 1380 | - * synchronize_rcu() before returning from this function (either in case | |
| 1381 | - * of error or after replacing it with heap-allocated mask in | |
| 1382 | - * fl_create_new_mask()). | |
| 1386 | + * -EAGAIN because mask's refcnt is zero. | |
| 1383 | 1387 | */ |
| 1384 | 1388 | fnew->mask = rhashtable_lookup_get_insert_fast(&head->ht, |
| 1385 | 1389 | &mask->ht_node, |
| ... | ... | @@ -1414,8 +1418,6 @@ |
| 1414 | 1418 | errout_cleanup: |
| 1415 | 1419 | rhashtable_remove_fast(&head->ht, &mask->ht_node, |
| 1416 | 1420 | mask_ht_params); |
| 1417 | - /* Wait until any potential concurrent users of mask are finished */ | |
| 1418 | - synchronize_rcu(); | |
| 1419 | 1421 | return ret; |
| 1420 | 1422 | } |
| 1421 | 1423 | |
| ... | ... | @@ -1644,7 +1646,7 @@ |
| 1644 | 1646 | *arg = fnew; |
| 1645 | 1647 | |
| 1646 | 1648 | kfree(tb); |
| 1647 | - kfree(mask); | |
| 1649 | + tcf_queue_work(&mask->rwork, fl_uninit_mask_free_work); | |
| 1648 | 1650 | return 0; |
| 1649 | 1651 | |
| 1650 | 1652 | errout_ht: |
| ... | ... | @@ -1664,7 +1666,7 @@ |
| 1664 | 1666 | errout_tb: |
| 1665 | 1667 | kfree(tb); |
| 1666 | 1668 | errout_mask_alloc: |
| 1667 | - kfree(mask); | |
| 1669 | + tcf_queue_work(&mask->rwork, fl_uninit_mask_free_work); | |
| 1668 | 1670 | errout_fold: |
| 1669 | 1671 | if (fold) |
| 1670 | 1672 | __fl_put(fold); |