tc/netem: loss gemodel options fixes

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.

---
 tc/q_netem.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

index c83e301..8abe07f 100644
@@ -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

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, [email protected]

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

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 <[email protected]>
To: Hagen Paul Pfeifer <[email protected]>
cc: [email protected], [email protected],
Stephen Hemminger <[email protected]>
Subject: Re: [PATCH iproute2] tc/netem: loss gemodel options fixes
Date: Thu, 15 May 2014 12:46:39 -0700

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, [email protected]

Great Jay, I did not read that you tested/validated your patch.

Could you get review of original author of this loss model?
Stefano Salsano <[email protected]>