| Message ID | 1392173895-5012-5-git-send-email-yangyingliang@huawei.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Yang Yingliang <yangyingliang@huawei.com> Date: Wed, 12 Feb 2014 10:58:15 +0800 > spin_(un)lock_bh(root_lock) is same as sch_tree_(un)lock. > > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> ... > @@ -684,11 +683,9 @@ static int get_dist_table(struct Qdisc *sch, const struct nlattr *attr) > for (i = 0; i < n; i++) > d->table[i] = data[i]; > > - root_lock = qdisc_root_sleeping_lock(sch); > - > - spin_lock_bh(root_lock); > + sch_tree_lock(sch); > swap(q->delay_dist, d); > - spin_unlock_bh(root_lock); > + sch_tree_unlock(sch); > > dist_free(d); > return 0; This is more expensive than the existing code. We will now calculate qdisc_root_sleeping_lock() twice which is at least two pointer dereferences each. Without explicitly open-coding this, the compiler cannot cache the result, because the spin lock operations have memory barriers (if implemented inline) or are considered to potentially modify all memory (if implemented as function calls). -- 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 2014/2/14 6:46, David Miller wrote: > From: Yang Yingliang <yangyingliang@huawei.com> > Date: Wed, 12 Feb 2014 10:58:15 +0800 > >> spin_(un)lock_bh(root_lock) is same as sch_tree_(un)lock. >> >> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > ... >> @@ -684,11 +683,9 @@ static int get_dist_table(struct Qdisc *sch, const struct nlattr *attr) >> for (i = 0; i < n; i++) >> d->table[i] = data[i]; >> >> - root_lock = qdisc_root_sleeping_lock(sch); >> - >> - spin_lock_bh(root_lock); >> + sch_tree_lock(sch); >> swap(q->delay_dist, d); >> - spin_unlock_bh(root_lock); >> + sch_tree_unlock(sch); >> >> dist_free(d); >> return 0; > > This is more expensive than the existing code. > > We will now calculate qdisc_root_sleeping_lock() twice which is at > least two pointer dereferences each. > > Without explicitly open-coding this, the compiler cannot cache the > result, because the spin lock operations have memory barriers (if > implemented inline) or are considered to potentially modify all memory > (if implemented as function calls). > > OK, thanks! I'll send v2 without this patch. -- 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/net/sched/sch_netem.c b/net/sched/sch_netem.c index 4fced67..62811d5 100644 --- a/net/sched/sch_netem.c +++ b/net/sched/sch_netem.c @@ -665,7 +665,6 @@ static int get_dist_table(struct Qdisc *sch, const struct nlattr *attr) struct netem_sched_data *q = qdisc_priv(sch); size_t n = nla_len(attr)/sizeof(__s16); const __s16 *data = nla_data(attr); - spinlock_t *root_lock; struct disttable *d; int i; size_t s; @@ -684,11 +683,9 @@ static int get_dist_table(struct Qdisc *sch, const struct nlattr *attr) for (i = 0; i < n; i++) d->table[i] = data[i]; - root_lock = qdisc_root_sleeping_lock(sch); - - spin_lock_bh(root_lock); + sch_tree_lock(sch); swap(q->delay_dist, d); - spin_unlock_bh(root_lock); + sch_tree_unlock(sch); dist_free(d); return 0;
spin_(un)lock_bh(root_lock) is same as sch_tree_(un)lock. Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> --- net/sched/sch_netem.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)