* [PATCH iproute2] tc/netem: loss gemodel options fixes
@ 2014-05-10 20:34 Jay Vosburgh
[not found] ` <CAPh34mf4ahPyTDf-tEhsNKSFBL6GYjES9+TTvDjhMf2YLTr04Q@mail.gmail.com>
2014-05-15 21:03 ` Stephen Hemminger
0 siblings, 2 replies; 7+ messages in thread
From: Jay Vosburgh @ 2014-05-10 20:34 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, netem
First, the default value for 1-k is documented as being 0, but is
currently being set to 1. (100%). This causes all packets to be dropped
in the good state if 1-k is not explicitly specified. Fix this by setting
the default to 0.
Second, the 1-h option is parsed correctly, however, the kernel is
expecting "h", not 1-h. Fix this by inverting the "1-h" percentage before
sending to and after receiving from the kernel. This does change the
behavior, but makes it consistent with the netem documentation and the
literature on the Gilbert-Elliot model, which refer to "1-h" and "1-k,"
not "h" or "k" directly.
Last, fix a minor formatting issue for the options reporting.
Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
---
tc/q_netem.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/tc/q_netem.c b/tc/q_netem.c
index c83e301..8abe07f 100644
--- a/tc/q_netem.c
+++ b/tc/q_netem.c
@@ -307,7 +307,7 @@ static int netem_parse_opt(struct qdisc_util *qu, int argc, char **argv,
/* set defaults */
set_percent(&gemodel.r, 1.);
set_percent(&gemodel.h, 0);
- set_percent(&gemodel.k1, 1.);
+ set_percent(&gemodel.k1, 0);
loss_type = NETEM_LOSS_GE;
if (!NEXT_IS_NUMBER())
@@ -325,6 +325,10 @@ static int netem_parse_opt(struct qdisc_util *qu, int argc, char **argv,
explain1("loss gemodel h");
return -1;
}
+ /* netem option is "1-h" but kernel
+ * expects "h".
+ */
+ gemodel.h = max_percent_value - gemodel.h;
if (!NEXT_IS_NUMBER())
continue;
@@ -625,10 +629,11 @@ static int netem_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
}
if (gemodel) {
- fprintf(f, "loss gemodel p %s",
+ fprintf(f, " loss gemodel p %s",
sprint_percent(gemodel->p, b1));
fprintf(f, " r %s", sprint_percent(gemodel->r, b1));
- fprintf(f, " 1-h %s", sprint_percent(gemodel->h, b1));
+ fprintf(f, " 1-h %s", sprint_percent(max_percent_value -
+ gemodel->h, b1));
fprintf(f, " 1-k %s", sprint_percent(gemodel->k1, b1));
}
--
1.8.3.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH iproute2] tc/netem: loss gemodel options fixes
[not found] ` <CAPh34mf4ahPyTDf-tEhsNKSFBL6GYjES9+TTvDjhMf2YLTr04Q@mail.gmail.com>
@ 2014-05-15 19:46 ` Jay Vosburgh
2014-08-04 19:37 ` Stephen Hemminger
0 siblings, 1 reply; 7+ messages in thread
From: Jay Vosburgh @ 2014-05-15 19:46 UTC (permalink / raw)
To: Hagen Paul Pfeifer; +Cc: netdev, netem, Stephen Hemminger
Hagen Paul Pfeifer <hagen@jauu.net> wrote:
>Stephen, tomorrow I will take a look at Jay's patches.
Just to make it clear what I believe is incorrect with regards
to the h and 1-h part:
net/sched/sch_netem.c:
[...]
/* 4-states and Gilbert-Elliot models */
u32 a1; /* p13 for 4-states or p for GE */
u32 a2; /* p31 for 4-states or r for GE */
u32 a3; /* p32 for 4-states or h for GE */
u32 a4; /* p14 for 4-states or 1-k for GE */
[...]
Note that a3 is "h for GE" vs a4 is "1-k for GE". Also, in
the actual drop function:
static bool loss_gilb_ell(struct netem_sched_data *q)
[...]
case GOOD_STATE:
[...]
if (prandom_u32() < clg->a4)
return true;
break;
case BAD_STATE:
[...]
if (prandom_u32() > clg->a3)
return true;
[...]
The test for clg->a3 is inverted as compared to the test for
clg->a4. Hence, the kernel is using "h," not "1-h," and therefore tc
should pass in the value for h instead of 1-h as it does currently.
-J
>On May 10, 2014 10:35 PM, "Jay Vosburgh" <jay.vosburgh@canonical.com>
>wrote:
>
> First, the default value for 1-k is documented as being 0, but is
> currently being set to 1. (100%). This causes all packets to be
> dropped
> in the good state if 1-k is not explicitly specified. Fix this by
> setting
> the default to 0.
>
> Second, the 1-h option is parsed correctly, however, the kernel is
> expecting "h", not 1-h. Fix this by inverting the "1-h" percentage
> before
> sending to and after receiving from the kernel. This does change the
> behavior, but makes it consistent with the netem documentation and the
> literature on the Gilbert-Elliot model, which refer to "1-h" and
> "1-k,"
> not "h" or "k" directly.
>
> Last, fix a minor formatting issue for the options reporting.
>
> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
> ---
> tc/q_netem.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/tc/q_netem.c b/tc/q_netem.c
> index c83e301..8abe07f 100644
> --- a/tc/q_netem.c
> +++ b/tc/q_netem.c
> @@ -307,7 +307,7 @@ static int netem_parse_opt(struct qdisc_util *qu,
> int argc, char **argv,
> /* set defaults */
> set_percent(&gemodel.r, 1.);
> set_percent(&gemodel.h, 0);
> - set_percent(&gemodel.k1, 1.);
> + set_percent(&gemodel.k1, 0);
> loss_type = NETEM_LOSS_GE;
>
> if (!NEXT_IS_NUMBER())
> @@ -325,6 +325,10 @@ static int netem_parse_opt(struct qdisc_util *qu,
> int argc, char **argv,
> explain1("loss gemodel h");
> return -1;
> }
> + /* netem option is "1-h" but kernel
> + * expects "h".
> + */
> + gemodel.h = max_percent_value - gemodel.h;
>
> if (!NEXT_IS_NUMBER())
> continue;
> @@ -625,10 +629,11 @@ static int netem_print_opt(struct qdisc_util
> *qu, FILE *f, struct rtattr *opt)
> }
>
> if (gemodel) {
> - fprintf(f, "loss gemodel p %s",
> + fprintf(f, " loss gemodel p %s",
> sprint_percent(gemodel->p, b1));
> fprintf(f, " r %s", sprint_percent(gemodel->r, b1));
> - fprintf(f, " 1-h %s", sprint_percent(gemodel->h, b1));
> + fprintf(f, " 1-h %s", sprint_percent(max_percent_value -
> + gemodel->h, b1));
> fprintf(f, " 1-k %s", sprint_percent(gemodel->k1, b1));
> }
>
> --
> 1.8.3.2
---
-Jay Vosburgh, jay.vosburgh@canonical.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH iproute2] tc/netem: loss gemodel options fixes
2014-05-10 20:34 [PATCH iproute2] tc/netem: loss gemodel options fixes Jay Vosburgh
[not found] ` <CAPh34mf4ahPyTDf-tEhsNKSFBL6GYjES9+TTvDjhMf2YLTr04Q@mail.gmail.com>
@ 2014-05-15 21:03 ` Stephen Hemminger
1 sibling, 0 replies; 7+ messages in thread
From: Stephen Hemminger @ 2014-05-15 21:03 UTC (permalink / raw)
To: Jay Vosburgh; +Cc: netdev, netem
On Sat, 10 May 2014 13:34:58 -0700
Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
>
> First, the default value for 1-k is documented as being 0, but is
> currently being set to 1. (100%). This causes all packets to be dropped
> in the good state if 1-k is not explicitly specified. Fix this by setting
> the default to 0.
>
> Second, the 1-h option is parsed correctly, however, the kernel is
> expecting "h", not 1-h. Fix this by inverting the "1-h" percentage before
> sending to and after receiving from the kernel. This does change the
> behavior, but makes it consistent with the netem documentation and the
> literature on the Gilbert-Elliot model, which refer to "1-h" and "1-k,"
> not "h" or "k" directly.
>
> Last, fix a minor formatting issue for the options reporting.
>
> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
Could you get review of original author of this loss model?
Stefano Salsano <stefano.salsano@uniroma2.it>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH iproute2] tc/netem: loss gemodel options fixes
2014-05-15 19:46 ` Jay Vosburgh
@ 2014-08-04 19:37 ` Stephen Hemminger
2014-08-05 8:09 ` Hagen Paul Pfeifer
0 siblings, 1 reply; 7+ messages in thread
From: Stephen Hemminger @ 2014-08-04 19:37 UTC (permalink / raw)
To: Jay Vosburgh; +Cc: Hagen Paul Pfeifer, netdev, netem
I went ahead and applied these. They make sense and got no response.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH iproute2] tc/netem: loss gemodel options fixes
2014-08-04 19:37 ` Stephen Hemminger
@ 2014-08-05 8:09 ` Hagen Paul Pfeifer
2014-08-05 18:29 ` Jay Vosburgh
0 siblings, 1 reply; 7+ messages in thread
From: Hagen Paul Pfeifer @ 2014-08-05 8:09 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Jay Vosburgh, netdev, netem
On 4 August 2014 21:37, Stephen Hemminger <stephen@networkplumber.org> wrote:
> I went ahead and applied these. They make sense and got no response.
Wait Stephen,
Jay do you compared your changes with the expected results? I mean did
you run tests that the Markov chain model is _now_ working correctly
(in all states)?
The setup will be easy: send 10000 packets, capture the packets and
'wc -l tcpdump -r trace.pcap' and compare to the expected number of
packets for a given markov state setup. Enough bugs here where the
should be no bugs at all. Some simple tests should be enough to get
rid of them.
Hagen
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH iproute2] tc/netem: loss gemodel options fixes
2014-08-05 8:09 ` Hagen Paul Pfeifer
@ 2014-08-05 18:29 ` Jay Vosburgh
2014-08-05 21:25 ` Hagen Paul Pfeifer
0 siblings, 1 reply; 7+ messages in thread
From: Jay Vosburgh @ 2014-08-05 18:29 UTC (permalink / raw)
To: Hagen Paul Pfeifer; +Cc: Stephen Hemminger, netdev, netem
Hagen Paul Pfeifer <hagen@jauu.net> wrote:
>On 4 August 2014 21:37, Stephen Hemminger <stephen@networkplumber.org> wrote:
>
>> I went ahead and applied these. They make sense and got no response.
>
>Wait Stephen,
>
>Jay do you compared your changes with the expected results? I mean did
>you run tests that the Markov chain model is _now_ working correctly
>(in all states)?
>
>The setup will be easy: send 10000 packets, capture the packets and
>'wc -l tcpdump -r trace.pcap' and compare to the expected number of
>packets for a given markov state setup. Enough bugs here where the
>should be no bugs at all. Some simple tests should be enough to get
>rid of them.
I did test the changes when I originally submitted them, yes.
The kernel code is unmodified; what the patch changed is
- the default for 1-k if not supplied as an option is documented
as 1-k=0, but was actually set to 1, i.e., drop everything in good state
if 1-k is not explicitly specified.
- convert "1-h" to "h" as the kernel expects. As I recall, I
originally noticed this when trying to specify small loss percentages in
the bad state, e.g., something like "netem loss gemodel 100 0 1" should
drop 1% in the bad state (1-h == 1%), but would instead drop 99%.
I also posted some additional analysis:
From: Jay Vosburgh <jay.vosburgh@canonical.com>
To: Hagen Paul Pfeifer <hagen@jauu.net>
cc: netdev@vger.kernel.org, netem@lists.linux-foundation.org,
Stephen Hemminger <stephen@networkplumber.org>
Subject: Re: [PATCH iproute2] tc/netem: loss gemodel options fixes
Date: Thu, 15 May 2014 12:46:39 -0700
Hagen Paul Pfeifer <hagen@jauu.net> wrote:
>Stephen, tomorrow I will take a look at Jay's patches.
Just to make it clear what I believe is incorrect with regards
to the h and 1-h part:
net/sched/sch_netem.c:
[...]
/* 4-states and Gilbert-Elliot models */
u32 a1; /* p13 for 4-states or p for GE */
u32 a2; /* p31 for 4-states or r for GE */
u32 a3; /* p32 for 4-states or h for GE */
u32 a4; /* p14 for 4-states or 1-k for GE */
[...]
Note that a3 is "h for GE" vs a4 is "1-k for GE". Also, in
the actual drop function:
static bool loss_gilb_ell(struct netem_sched_data *q)
[...]
case GOOD_STATE:
[...]
if (prandom_u32() < clg->a4)
return true;
break;
case BAD_STATE:
[...]
if (prandom_u32() > clg->a3)
return true;
[...]
The test for clg->a3 is inverted as compared to the test for
clg->a4. Hence, the kernel is using "h," not "1-h," and therefore tc
should pass in the value for h instead of 1-h as it does currently.
-J
---
-Jay Vosburgh, jay.vosburgh@canonical.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH iproute2] tc/netem: loss gemodel options fixes
2014-08-05 18:29 ` Jay Vosburgh
@ 2014-08-05 21:25 ` Hagen Paul Pfeifer
0 siblings, 0 replies; 7+ messages in thread
From: Hagen Paul Pfeifer @ 2014-08-05 21:25 UTC (permalink / raw)
To: Jay Vosburgh; +Cc: Stephen Hemminger, netdev, netem
Great Jay, I did not read that you tested/validated your patch.
Acked-by: Hagen Paul Pfeifer <hagen@jauu.net>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-08-05 21:25 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-10 20:34 [PATCH iproute2] tc/netem: loss gemodel options fixes Jay Vosburgh
[not found] ` <CAPh34mf4ahPyTDf-tEhsNKSFBL6GYjES9+TTvDjhMf2YLTr04Q@mail.gmail.com>
2014-05-15 19:46 ` Jay Vosburgh
2014-08-04 19:37 ` Stephen Hemminger
2014-08-05 8:09 ` Hagen Paul Pfeifer
2014-08-05 18:29 ` Jay Vosburgh
2014-08-05 21:25 ` Hagen Paul Pfeifer
2014-05-15 21:03 ` Stephen Hemminger
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.