diff mbox

Re: some tc commands fail on 2.6.29-rc6-git5, works on 2.6.28.7

Message ID 20090304233458.GA8303@ami.dom.local
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Jarek Poplawski March 4, 2009, 11:34 p.m. UTC
On Tue, Mar 03, 2009 at 08:50:28PM -0600, John Dykstra wrote:
> On Tue, 2009-03-03 at 23:10 +0000, Joao Correia wrote:
> > I have confirmed again that the exact same commands (as on the
> > original message), work flawlessly on 2.6.28.7 without a hitch, and,
> > AFAIK, work as intended. I have also tried the latest 2.6.29-rc6-git7
> > and it still fails like on git6.
> 
> I have also reproduced this on Linus' latest 2.6.29-rc6 tree, using tc
> version iproute2-ss071016 and a kernel config with all traffic control
> features enabled.
> 
> The error reported by tc comes from the kernel-level check added by:
> 
> 	commit c1b56878fb68e9c14070939ea4537ad4db79ffae
> 	Author: Stephen Hemminger <shemminger@vyatta.com>
> 	Date:   Tue Nov 25 21:14:06 2008 -0800
> 
>     	tc: policing requires a rate estimator
> 
>     Found that while trying average rate policing, it was possible to
>     request average rate policing without a rate estimator. This results
>     in no policing which is harmless but incorrect.
> 
>     Since policing could be setup in two steps, need to check
>     in the kernel.
> 
>     Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> I got the same error when using the tc version iproute2-ss090115 built
> this evening from the public git repository.
> 
>   -- John
> 

Very nice diagnose, thanks!
Jarek P.

PS: after upgrading iproute I couldn't reproduce this seemingly OK
test with older versions anymore...

------------------->
pkt_sched: act_police: Fix a rate estimator test.

A commit c1b56878fb68e9c14070939ea4537ad4db79ffae "tc: policing requires
a rate estimator" introduced a test which invalidates previously working
configs, based on examples from iproute2: doc/actions/actions-general.
This is too rigorous: a rate estimator is needed only when police's
"avrate" option is used.

Reported-by: Joao Correia <joaomiguelcorreia@gmail.com>
Diagnosed-by: John Dykstra <john.dykstra1@gmail.com>
Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
---

 net/sched/act_police.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

--
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

Comments

Joao Correia March 5, 2009, 12:32 a.m. UTC | #1
Hello

Confirmed working now on latest 2.6.29-rc6 git.

Thank you very much all for the prompt answer and solution, really impressive.

Back to work on the ultimate custom firewall *evil laugh*.

Cheers,
Joao Correia
Centro de Informatica
Universidade da Beira Interior
Portugal

On Wed, Mar 4, 2009 at 11:34 PM, Jarek Poplawski <jarkao2@gmail.com> wrote:
> On Tue, Mar 03, 2009 at 08:50:28PM -0600, John Dykstra wrote:
>> On Tue, 2009-03-03 at 23:10 +0000, Joao Correia wrote:
>> > I have confirmed again that the exact same commands (as on the
>> > original message), work flawlessly on 2.6.28.7 without a hitch, and,
>> > AFAIK, work as intended. I have also tried the latest 2.6.29-rc6-git7
>> > and it still fails like on git6.
>>
>> I have also reproduced this on Linus' latest 2.6.29-rc6 tree, using tc
>> version iproute2-ss071016 and a kernel config with all traffic control
>> features enabled.
>>
>> The error reported by tc comes from the kernel-level check added by:
>>
>>       commit c1b56878fb68e9c14070939ea4537ad4db79ffae
>>       Author: Stephen Hemminger <shemminger@vyatta.com>
>>       Date:   Tue Nov 25 21:14:06 2008 -0800
>>
>>       tc: policing requires a rate estimator
>>
>>     Found that while trying average rate policing, it was possible to
>>     request average rate policing without a rate estimator. This results
>>     in no policing which is harmless but incorrect.
>>
>>     Since policing could be setup in two steps, need to check
>>     in the kernel.
>>
>>     Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
>>     Signed-off-by: David S. Miller <davem@davemloft.net>
>>
>> I got the same error when using the tc version iproute2-ss090115 built
>> this evening from the public git repository.
>>
>>   -- John
>>
>
> Very nice diagnose, thanks!
> Jarek P.
>
> PS: after upgrading iproute I couldn't reproduce this seemingly OK
> test with older versions anymore...
>
> ------------------->
> pkt_sched: act_police: Fix a rate estimator test.
>
> A commit c1b56878fb68e9c14070939ea4537ad4db79ffae "tc: policing requires
> a rate estimator" introduced a test which invalidates previously working
> configs, based on examples from iproute2: doc/actions/actions-general.
> This is too rigorous: a rate estimator is needed only when police's
> "avrate" option is used.
>
> Reported-by: Joao Correia <joaomiguelcorreia@gmail.com>
> Diagnosed-by: John Dykstra <john.dykstra1@gmail.com>
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
> ---
>
>  net/sched/act_police.c |   13 ++++++-------
>  1 files changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/net/sched/act_police.c b/net/sched/act_police.c
> index 5c72a11..f8f047b 100644
> --- a/net/sched/act_police.c
> +++ b/net/sched/act_police.c
> @@ -183,13 +183,6 @@ override:
>                if (R_tab == NULL)
>                        goto failure;
>
> -               if (!est && (ret == ACT_P_CREATED ||
> -                            !gen_estimator_active(&police->tcf_bstats,
> -                                                  &police->tcf_rate_est))) {
> -                       err = -EINVAL;
> -                       goto failure;
> -               }
> -
>                if (parm->peakrate.rate) {
>                        P_tab = qdisc_get_rtab(&parm->peakrate,
>                                               tb[TCA_POLICE_PEAKRATE]);
> @@ -205,6 +198,12 @@ override:
>                                            &police->tcf_lock, est);
>                if (err)
>                        goto failure_unlock;
> +       } else if (tb[TCA_POLICE_AVRATE] &&
> +                  (ret == ACT_P_CREATED ||
> +                   !gen_estimator_active(&police->tcf_bstats,
> +                                         &police->tcf_rate_est))) {
> +               err = -EINVAL;
> +               goto failure_unlock;
>        }
>
>        /* No failure allowed after this point */
>
--
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
David Miller March 5, 2009, 1:38 a.m. UTC | #2
From: Joao Correia <joaomiguelcorreia@gmail.com>
Date: Thu, 5 Mar 2009 00:32:08 +0000

> Confirmed working now on latest 2.6.29-rc6 git.
> 
> Thank you very much all for the prompt answer and solution, really impressive.
> 
> Back to work on the ultimate custom firewall *evil laugh*.

Applied, thanks everyone.
--
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 mbox

Patch

diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 5c72a11..f8f047b 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -183,13 +183,6 @@  override:
 		if (R_tab == NULL)
 			goto failure;
 
-		if (!est && (ret == ACT_P_CREATED ||
-			     !gen_estimator_active(&police->tcf_bstats,
-						   &police->tcf_rate_est))) {
-			err = -EINVAL;
-			goto failure;
-		}
-
 		if (parm->peakrate.rate) {
 			P_tab = qdisc_get_rtab(&parm->peakrate,
 					       tb[TCA_POLICE_PEAKRATE]);
@@ -205,6 +198,12 @@  override:
 					    &police->tcf_lock, est);
 		if (err)
 			goto failure_unlock;
+	} else if (tb[TCA_POLICE_AVRATE] &&
+		   (ret == ACT_P_CREATED ||
+		    !gen_estimator_active(&police->tcf_bstats,
+					  &police->tcf_rate_est))) {
+		err = -EINVAL;
+		goto failure_unlock;
 	}
 
 	/* No failure allowed after this point */