Re: [Patch net] net_sched: move tcf_lock down after gen_replace_estimator()

[David Miller]

From: Yuval Shaia 
Date: Tue, 13 Jun 2017 23:40:41 +0300

> On Tue, Jun 13, 2017 at 01:36:24PM -0700, Cong Wang wrote:
>> Laura reported a sleep-in-atomic kernel warning inside
> 
> Since you added a Reported-by tag below i don't see a reason to
> specifically mention it in commit log message.

That doesn't make any sense at all.

Part of telling the story is saying that someone reported a specific
kind of issue, and here is the problem, and here is how we solved it.

Just because there's a reported-by tag doesn't mean it's superfluous.

Re: [Patch net] net_sched: move tcf_lock down after gen_replace_estimator()

[David Miller]

From: Cong Wang 
Date: Tue, 13 Jun 2017 13:36:24 -0700

> Laura reported a sleep-in-atomic kernel warning inside
> tcf_act_police_init() which calls gen_replace_estimator() with
> spinlock protection.
> 
> It is not necessary in this case, we already have RTNL lock here
> so it is enough to protect concurrent writers. For the reader,
> i.e. tcf_act_police(), it needs to make decision based on this
> rate estimator, in the worst case we drop more/less packets than
> necessary while changing the rate in parallel, it is still acceptable.
> 
> Reported-by: Laura Abbott 
> Reported-by: Nick Huber 
> Cc: Jamal Hadi Salim 
> Signed-off-by: Cong Wang 

Applied, thanks.

Re: [Patch net] net_sched: move tcf_lock down after gen_replace_estimator()

[Jamal Hadi Salim]

On 17-06-13 04:36 PM, Cong Wang wrote:

Laura reported a sleep-in-atomic kernel warning inside
tcf_act_police_init() which calls gen_replace_estimator() with
spinlock protection.

It is not necessary in this case, we already have RTNL lock here
so it is enough to protect concurrent writers. For the reader,
i.e. tcf_act_police(), it needs to make decision based on this
rate estimator, in the worst case we drop more/less packets than
necessary while changing the rate in parallel, it is still acceptable.

Reported-by: Laura Abbott 
Reported-by: Nick Huber 
Cc: Jamal Hadi Salim 
Signed-off-by: Cong Wang 
---
  net/sched/act_police.c | 8 +++-
  1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index f42008b..b062bc8 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -132,21 +132,21 @@ static int tcf_act_police_init(struct net *net, struct 
nlattr *nla,
}
}
  
-	spin_lock_bh(>tcf_lock);

if (est) {
err = gen_replace_estimator(>tcf_bstats, NULL,
>tcf_rate_est,
>tcf_lock,
NULL, est);
if (err)
-   goto failure_unlock;
+   goto failure;
} else if (tb[TCA_POLICE_AVRATE] &&
   (ret == ACT_P_CREATED ||
!gen_estimator_active(>tcf_rate_est))) {
err = -EINVAL;
-   goto failure_unlock;
+   goto failure;
}
  
+	spin_lock_bh(>tcf_lock);

/* No failure allowed after this point */
police->tcfp_mtu = parm->mtu;
if (police->tcfp_mtu == 0) {
@@ -192,8 +192,6 @@ static int tcf_act_police_init(struct net *net, struct 
nlattr *nla,
  
  	return ret;
  
-failure_unlock:

-   spin_unlock_bh(>tcf_lock);
  failure:
qdisc_put_rtab(P_tab);
qdisc_put_rtab(R_tab);




Looks good to me.

Acked-by: Jamal Hadi Salim 

cheers,
jamal

Re: [Patch net] net_sched: move tcf_lock down after gen_replace_estimator()

[Cong Wang]

On Tue, Jun 13, 2017 at 1:40 PM, Yuval Shaia  wrote:
> On Tue, Jun 13, 2017 at 01:36:24PM -0700, Cong Wang wrote:
>> Laura reported a sleep-in-atomic kernel warning inside
>
> Since you added a Reported-by tag below i don't see a reason to
> specifically mention it in commit log message.

If you have faith in this, feel free to update netdev-FAQ.txt to save
your time and others' time in the future.

Thanks.

Re: [Patch net] net_sched: move tcf_lock down after gen_replace_estimator()

[Yuval Shaia]

On Tue, Jun 13, 2017 at 01:36:24PM -0700, Cong Wang wrote:
> Laura reported a sleep-in-atomic kernel warning inside

Since you added a Reported-by tag below i don't see a reason to
specifically mention it in commit log message.

> tcf_act_police_init() which calls gen_replace_estimator() with
> spinlock protection.
> 
> It is not necessary in this case, we already have RTNL lock here
> so it is enough to protect concurrent writers. For the reader,
> i.e. tcf_act_police(), it needs to make decision based on this
> rate estimator, in the worst case we drop more/less packets than
> necessary while changing the rate in parallel, it is still acceptable.
> 
> Reported-by: Laura Abbott 
> Reported-by: Nick Huber 
> Cc: Jamal Hadi Salim 
> Signed-off-by: Cong Wang 
> ---
>  net/sched/act_police.c | 8 +++-
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/net/sched/act_police.c b/net/sched/act_police.c
> index f42008b..b062bc8 100644
> --- a/net/sched/act_police.c
> +++ b/net/sched/act_police.c
> @@ -132,21 +132,21 @@ static int tcf_act_police_init(struct net *net, struct 
> nlattr *nla,
>   }
>   }
>  
> - spin_lock_bh(>tcf_lock);
>   if (est) {
>   err = gen_replace_estimator(>tcf_bstats, NULL,
>   >tcf_rate_est,
>   >tcf_lock,
>   NULL, est);
>   if (err)
> - goto failure_unlock;
> + goto failure;
>   } else if (tb[TCA_POLICE_AVRATE] &&
>  (ret == ACT_P_CREATED ||
>   !gen_estimator_active(>tcf_rate_est))) {
>   err = -EINVAL;
> - goto failure_unlock;
> + goto failure;
>   }
>  
> + spin_lock_bh(>tcf_lock);
>   /* No failure allowed after this point */
>   police->tcfp_mtu = parm->mtu;
>   if (police->tcfp_mtu == 0) {
> @@ -192,8 +192,6 @@ static int tcf_act_police_init(struct net *net, struct 
> nlattr *nla,
>  
>   return ret;
>  
> -failure_unlock:
> - spin_unlock_bh(>tcf_lock);
>  failure:
>   qdisc_put_rtab(P_tab);
>   qdisc_put_rtab(R_tab);
> -- 
> 2.5.5
> 

[Patch net] net_sched: move tcf_lock down after gen_replace_estimator()

[Cong Wang]

Laura reported a sleep-in-atomic kernel warning inside
tcf_act_police_init() which calls gen_replace_estimator() with
spinlock protection.

It is not necessary in this case, we already have RTNL lock here
so it is enough to protect concurrent writers. For the reader,
i.e. tcf_act_police(), it needs to make decision based on this
rate estimator, in the worst case we drop more/less packets than
necessary while changing the rate in parallel, it is still acceptable.

Reported-by: Laura Abbott 
Reported-by: Nick Huber 
Cc: Jamal Hadi Salim 
Signed-off-by: Cong Wang 
---
 net/sched/act_police.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index f42008b..b062bc8 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -132,21 +132,21 @@ static int tcf_act_police_init(struct net *net, struct 
nlattr *nla,
}
}
 
-   spin_lock_bh(>tcf_lock);
if (est) {
err = gen_replace_estimator(>tcf_bstats, NULL,
>tcf_rate_est,
>tcf_lock,
NULL, est);
if (err)
-   goto failure_unlock;
+   goto failure;
} else if (tb[TCA_POLICE_AVRATE] &&
   (ret == ACT_P_CREATED ||
!gen_estimator_active(>tcf_rate_est))) {
err = -EINVAL;
-   goto failure_unlock;
+   goto failure;
}
 
+   spin_lock_bh(>tcf_lock);
/* No failure allowed after this point */
police->tcfp_mtu = parm->mtu;
if (police->tcfp_mtu == 0) {
@@ -192,8 +192,6 @@ static int tcf_act_police_init(struct net *net, struct 
nlattr *nla,
 
return ret;
 
-failure_unlock:
-   spin_unlock_bh(>tcf_lock);
 failure:
qdisc_put_rtab(P_tab);
qdisc_put_rtab(R_tab);
-- 
2.5.5