All of lore.kernel.org
 help / color / mirror / Atom feed
* pktgen IP address stepping
@ 2010-12-16 17:28 Kristian Larsson
  2010-12-16 17:44 ` David Miller
  2010-12-16 18:02 ` Joe Perches
  0 siblings, 2 replies; 12+ messages in thread
From: Kristian Larsson @ 2010-12-16 17:28 UTC (permalink / raw)
  To: David S. Miller; +Cc: netdev

[-- Attachment #1: Type: text/plain, Size: 3270 bytes --]

Hello David (and list)!

First of all, I'm hoping I'm sending this to the right person / list,
I've never submitted nor written a patch for the Linux kernel before,
so please bare with me.


This patch adds the ability to set the step rate at which the source
or destination IP address is increased when src_min / src_max or
dst_min / dst_max is used with pktgen.

Usage is simple, two new parameters:
  src_step X
  dst_step X
where X is a positive integer (or actually an unsigned long long).

This is useful for network testing when you want to send packets to
large parts of the Internet.

In addition to adding stepping support, I've changed the init value of
current source / destination address in the code to the max. The
current kernel sets it to the min value, but before the first packet
is sent out, the value is increased. By setting it to max, and we try
to increase it, it will wrap and thus actually start at the min value.
I find this more intuitive and to follow the principle of least
astonishement - I'd actually go as far as say the old behaviour is a
bug. Anyway, there are also some formatting changes and editorial
nitpicks.  Of course, src / dst step has been added to the output of
the proc interface.


My tests involved performance measurements of routers, specifically in
the area of convergence times. Most larger routers implement their FIB
in a TCAM, SRAM or something similarly fast and deterministic. While
reads, ie lookups are often very fast, updates are often quite slow.
My testing show some platforms rewriting some 2000 prefixes per
seconds. So basically, what I wanted to test was just how routers
rewrite their FIB and so I needed to send a packet to every prefix they
have stored in FIB and receive it on one of two output interfaces - the
first if the reroute has not yet happened and on the second interface
if it has. Since I'm dealing with Internet core routers that means I
have a full BGP table (ie some 330k prefix as of late 2010) and I thus
want to send one packet, or 330k packets per second, to each and every
prefix. In addition, it would require to me to input a list of
prefixes to pktgen somehow as the prefixes certainly aren't evenly
spread out (ie, not evenly sized). Instead of implementing that, I
went for an approximation, sending a packet to every Nth address,
which in my case is 65535, or every /16. This gives a pretty good
approximate of the entire routing table, at least good enough for my
tests.

Tests will be posted on http://labs.spritelink.net for those
interested. It will probably be until the beginning of next year
before anything is posted since most of the testing is performed at my
employer, Tele2, and we need to wrap it all up first.

The changes included in this patch was developed during my own (and
not my employers) time and I give permission to place it under whatever
license needed for inclusion in the Linux kernel. pktgen.c appeared to
include some list of changes at the top so I added myself, if this is
too small a change or whatever, that can of course be removed.

Looking forward to any feedback and / or suggestions.

Kind regards,
   Kristian.

-- 
Kristian Larsson                                        KLL-RIPE
+46 704 264511			              kll@spritelink.net

[-- Attachment #2: linux-pktgen-step.patch --]
[-- Type: text/x-diff, Size: 4299 bytes --]

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 2953b2a..391cecc 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -114,6 +114,9 @@
  * Fixed src_mac command to set source mac of packet to value specified in
  * command by Adit Ranadive <adit.262@gmail.com>
  *
+ * Added support for increasing src / dst ip by values larger than one (1) when
+ * doing src or dst min/max by Kristian Larsson <kll@spritelink.net>
+ *
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -288,6 +291,10 @@ struct pktgen_dev {
 	char src_min[IP_NAME_SZ];	/* IP, ie 1.2.3.4 */
 	char src_max[IP_NAME_SZ];	/* IP, ie 1.2.3.4 */
 
+	unsigned long long dst_step;
+	unsigned long long src_step;	/* Step up (ie increase) IP src / dst by this
+					   much. 1 by default. */
+
 	struct in6_addr in6_saddr;
 	struct in6_addr in6_daddr;
 	struct in6_addr cur_in6_daddr;
@@ -572,11 +579,11 @@ static int pktgen_if_show(struct seq_file *seq, void *v)
 
 	} else {
 		seq_printf(seq,
-			   "     dst_min: %s  dst_max: %s\n",
-			   pkt_dev->dst_min, pkt_dev->dst_max);
+			   "     dst_min: %s  dst_max: %s  dst_step: %llu\n",
+			   pkt_dev->dst_min, pkt_dev->dst_max, pkt_dev->dst_step);
 		seq_printf(seq,
-			   "        src_min: %s  src_max: %s\n",
-			   pkt_dev->src_min, pkt_dev->src_max);
+			   "     src_min: %s  src_max: %s  src_step: %llu\n",
+			   pkt_dev->src_min, pkt_dev->src_max, pkt_dev->src_step);
 	}
 
 	seq_puts(seq, "     src_mac: ");
@@ -1359,6 +1366,21 @@ static ssize_t pktgen_if_write(struct file *file,
 		sprintf(pg_result, "OK: dst6_max=%s", buf);
 		return count;
 	}
+	if (!strcmp(name, "dst_step")) {
+		len = num_arg(&user_buffer[i], 10, &value);
+		if (len < 0)
+			return len;
+
+		i += len;
+		if (!value)
+			return len;
+		pkt_dev->dst_step = value;
+		if (debug)
+			pr_info("pktgen: dst_step set to: %llu\n", pkt_dev->dst_step);
+
+		sprintf(pg_result, "OK: dst_step=%llu", pkt_dev->dst_step);
+		return count;
+	}
 	if (!strcmp(name, "src6")) {
 		len = strn_len(&user_buffer[i], sizeof(buf) - 1);
 		if (len < 0)
@@ -1424,6 +1446,21 @@ static ssize_t pktgen_if_write(struct file *file,
 		sprintf(pg_result, "OK: src_max=%s", pkt_dev->src_max);
 		return count;
 	}
+	if (!strcmp(name, "src_step")) {
+		len = num_arg(&user_buffer[i], 10, &value);
+		if (len < 0)
+			return len;
+
+		i += len;
+		if (!value)
+			return len;
+		pkt_dev->src_step = value;
+		if (debug)
+			pr_info("pktgen: src_step set to: %llu\n", pkt_dev->src_step);
+
+		sprintf(pg_result, "OK: src_step=%llu", pkt_dev->src_step);
+		return count;
+	}
 	if (!strcmp(name, "dst_mac")) {
 		char *v = valstr;
 		unsigned char old_dmac[ETH_ALEN];
@@ -2160,8 +2197,17 @@ static void pktgen_setup_inject(struct pktgen_dev *pkt_dev)
 	/* Initialize current values. */
 	pkt_dev->cur_dst_mac_offset = 0;
 	pkt_dev->cur_src_mac_offset = 0;
-	pkt_dev->cur_saddr = pkt_dev->saddr_min;
-	pkt_dev->cur_daddr = pkt_dev->daddr_min;
+
+	/*
+	 * Setting init value of current src / dst addr to max value seems
+	 * backwards, but it's not, it is increased for every run, including
+	 * the first one and so setting it to min value means we never actually
+	 * use the min value as it will be increased before we send the first
+	 * packet. Max on the other hand means we wrap on first run and thus
+	 * send first packet with min value.
+	 */
+	pkt_dev->cur_saddr = pkt_dev->saddr_max;
+	pkt_dev->cur_daddr = pkt_dev->daddr_max;
 	pkt_dev->cur_udp_dst = pkt_dev->udp_dst_min;
 	pkt_dev->cur_udp_src = pkt_dev->udp_src_min;
 	pkt_dev->nflows = 0;
@@ -2414,7 +2460,7 @@ static void mod_cur_headers(struct pktgen_dev *pkt_dev)
 				t = random32() % (imx - imn) + imn;
 			else {
 				t = ntohl(pkt_dev->cur_saddr);
-				t++;
+				t += pkt_dev->src_step;
 				if (t > imx)
 					t = imn;
 
@@ -2446,7 +2492,7 @@ static void mod_cur_headers(struct pktgen_dev *pkt_dev)
 					pkt_dev->cur_daddr = s;
 				} else {
 					t = ntohl(pkt_dev->cur_daddr);
-					t++;
+					t += pkt_dev->dst_step;
 					if (t > imx) {
 						t = imn;
 					}
@@ -3752,6 +3798,8 @@ static int pktgen_add_device(struct pktgen_thread *t, const char *ifname)
 	pkt_dev->udp_src_max = 9;
 	pkt_dev->udp_dst_min = 9;
 	pkt_dev->udp_dst_max = 9;
+	pkt_dev->dst_step = 1;
+	pkt_dev->src_step = 1;
 
 	pkt_dev->vlan_p = 0;
 	pkt_dev->vlan_cfi = 0;

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: pktgen IP address stepping
  2010-12-16 17:28 pktgen IP address stepping Kristian Larsson
@ 2010-12-16 17:44 ` David Miller
  2010-12-16 18:27   ` Kristian Larsson
  2010-12-16 18:02 ` Joe Perches
  1 sibling, 1 reply; 12+ messages in thread
From: David Miller @ 2010-12-16 17:44 UTC (permalink / raw)
  To: kristian; +Cc: netdev

From: Kristian Larsson <kristian@spritelink.net>
Date: Thu, 16 Dec 2010 18:28:00 +0100

> Looking forward to any feedback and / or suggestions.

If you need a huge type for the step values in order to handle ipv6
addresses or similar, used a fixed sized type such as "u64" instead of
"unsigned long long".

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: pktgen IP address stepping
  2010-12-16 17:28 pktgen IP address stepping Kristian Larsson
  2010-12-16 17:44 ` David Miller
@ 2010-12-16 18:02 ` Joe Perches
  2010-12-16 18:30   ` Kristian Larsson
  2010-12-16 21:11   ` Kristian Larsson
  1 sibling, 2 replies; 12+ messages in thread
From: Joe Perches @ 2010-12-16 18:02 UTC (permalink / raw)
  To: Kristian Larsson; +Cc: David S. Miller, netdev

On Thu, 2010-12-16 at 18:28 +0100, Kristian Larsson wrote:
> This patch adds the ability to set the step rate at which the source
> or destination IP address is increased when src_min / src_max or
> dst_min / dst_max is used with pktgen.
> Usage is simple, two new parameters:
>   src_step X
>   dst_step X
> where X is a positive integer (or actually an unsigned long long).
[]
> Looking forward to any feedback and / or suggestions.

I suggest using actual ipv4 addresses (and ipv6 if you ever
need it) as increment/mask.

For your current use, __u32 would work fine.

trivia:

pr_<foo> calls are already prefixed with pktgen via pr_fmt,
you don't need to add it to the format string.



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: pktgen IP address stepping
  2010-12-16 17:44 ` David Miller
@ 2010-12-16 18:27   ` Kristian Larsson
  2010-12-16 18:34     ` David Miller
  0 siblings, 1 reply; 12+ messages in thread
From: Kristian Larsson @ 2010-12-16 18:27 UTC (permalink / raw)
  To: David Miller; +Cc: netdev

On Thu, Dec 16, 2010 at 09:44:26AM -0800, David Miller wrote:
> From: Kristian Larsson <kristian@spritelink.net>
> Date: Thu, 16 Dec 2010 18:28:00 +0100
> 
> > Looking forward to any feedback and / or suggestions.
> 
> If you need a huge type for the step values in order to handle ipv6
> addresses or similar, used a fixed sized type such as "u64" instead of
> "unsigned long long".

It is not developed with IPv6 in mind, it actually doesn't touch
those parts of the code at all. Stepping through the IPv6 space
and incrementing by what would equal one /64 at a time is a
painfully slow process. We'd need all 128 bits there to make it
useful - an alternative would be to set the step length by prefix
mask instead, ie src6_step 16 would step through by increasing
one /16 at a time.

For IPv4 we could settle for less, something like 32 bits!? ;)

And given your comment I suppose u32 is a better choice than
"unsigned long", right? 

I was actually looking at the IPv6 part and noticed how it
appears to lack the option to set a min and max address, only
random addresses. My intention was to write a patch for that too,
though later on and if I'm able :)

Kind regards,
   Kristian.

-- 
Kristian Larsson                                        KLL-RIPE
+46 704 264511			              kll@spritelink.net

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: pktgen IP address stepping
  2010-12-16 18:02 ` Joe Perches
@ 2010-12-16 18:30   ` Kristian Larsson
  2010-12-16 19:02     ` [PATCH net-next] pktgen: Remove unnecessary prefix from pr_<level> Joe Perches
  2010-12-16 19:18     ` pktgen IP address stepping Joe Perches
  2010-12-16 21:11   ` Kristian Larsson
  1 sibling, 2 replies; 12+ messages in thread
From: Kristian Larsson @ 2010-12-16 18:30 UTC (permalink / raw)
  To: Joe Perches; +Cc: David S. Miller, netdev

On Thu, Dec 16, 2010 at 10:02:12AM -0800, Joe Perches wrote:
> On Thu, 2010-12-16 at 18:28 +0100, Kristian Larsson wrote:
> > This patch adds the ability to set the step rate at which the source
> > or destination IP address is increased when src_min / src_max or
> > dst_min / dst_max is used with pktgen.
> > Usage is simple, two new parameters:
> >   src_step X
> >   dst_step X
> > where X is a positive integer (or actually an unsigned long long).
> []
> > Looking forward to any feedback and / or suggestions.
> 
> I suggest using actual ipv4 addresses (and ipv6 if you ever
> need it) as increment/mask.

Not sure I follow you, should the stepping be specified as
0.0.0.213 if I would like to increase with 213 or 0.0.255.255 if
I want to increase by a /16 at a time?

> For your current use, __u32 would work fine.

Yepp, I'll have a look at that once I get home :)
 

> trivia:
> 
> pr_<foo> calls are already prefixed with pktgen via pr_fmt,
> you don't need to add it to the format string.

Same thing here, will look over it. I think I copied most of this
from some other part of pktgen.c :)

Kind regards,
    Kristian.

-- 
Kristian Larsson                                        KLL-RIPE
+46 704 264511			                      kll@spritelink.net

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: pktgen IP address stepping
  2010-12-16 18:27   ` Kristian Larsson
@ 2010-12-16 18:34     ` David Miller
  0 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2010-12-16 18:34 UTC (permalink / raw)
  To: kristian; +Cc: netdev

From: Kristian Larsson <kristian@spritelink.net>
Date: Thu, 16 Dec 2010 19:27:24 +0100

> And given your comment I suppose u32 is a better choice than
> "unsigned long", right? 

Yes.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH net-next] pktgen: Remove unnecessary prefix from pr_<level>
  2010-12-16 18:30   ` Kristian Larsson
@ 2010-12-16 19:02     ` Joe Perches
  2010-12-20 18:30       ` David Miller
  2010-12-16 19:18     ` pktgen IP address stepping Joe Perches
  1 sibling, 1 reply; 12+ messages in thread
From: Joe Perches @ 2010-12-16 19:02 UTC (permalink / raw)
  To: Kristian Larsson; +Cc: David S. Miller, netdev

Remove "pktgen: " prefix string from one pr_info.
pr_fmt adds it, so this is a duplicate.

Signed-off-by: Joe Perches <joe@perches.com>

---

> > trivia:
> > pr_<foo> calls are already prefixed with pktgen via pr_fmt,
> > you don't need to add it to the format string.
> Same thing here, will look over it. I think I copied most of this
> from some other part of pktgen.c :)

 net/core/pktgen.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 18fe20d..a9e7fc4 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -3553,8 +3553,7 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
 		break;
 	default: /* Drivers are not supposed to return other values! */
 		if (net_ratelimit())
-			pr_info("pktgen: %s xmit error: %d\n",
-				pkt_dev->odevname, ret);
+			pr_info("%s xmit error: %d\n", pkt_dev->odevname, ret);
 		pkt_dev->errors++;
 		/* fallthru */
 	case NETDEV_TX_LOCKED:



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: pktgen IP address stepping
  2010-12-16 18:30   ` Kristian Larsson
  2010-12-16 19:02     ` [PATCH net-next] pktgen: Remove unnecessary prefix from pr_<level> Joe Perches
@ 2010-12-16 19:18     ` Joe Perches
  2010-12-16 20:34       ` Kristian Larsson
  1 sibling, 1 reply; 12+ messages in thread
From: Joe Perches @ 2010-12-16 19:18 UTC (permalink / raw)
  To: Kristian Larsson; +Cc: David S. Miller, netdev

On Thu, 2010-12-16 at 19:30 +0100, Kristian Larsson wrote:
> > I suggest using actual ipv4 addresses (and ipv6 if you ever
> > need it) as increment/mask.
> Not sure I follow you, should the stepping be specified as
> 0.0.0.213 if I would like to increase with 213 or 0.0.255.255 if
> I want to increase by a /16 at a time?

More like:

Initial IPv4 address:	0.0.0.1
Step:			0.1.0.0
Mask:			127.0.0.1

addr = (addr + step) & mask;

Cycles through all /16's below 128




^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: pktgen IP address stepping
  2010-12-16 19:18     ` pktgen IP address stepping Joe Perches
@ 2010-12-16 20:34       ` Kristian Larsson
  0 siblings, 0 replies; 12+ messages in thread
From: Kristian Larsson @ 2010-12-16 20:34 UTC (permalink / raw)
  To: Joe Perches; +Cc: David S. Miller, netdev

On Thu, Dec 16, 2010 at 11:18:52AM -0800, Joe Perches wrote:
> On Thu, 2010-12-16 at 19:30 +0100, Kristian Larsson wrote:
> > > I suggest using actual ipv4 addresses (and ipv6 if you ever
> > > need it) as increment/mask.
> > Not sure I follow you, should the stepping be specified as
> > 0.0.0.213 if I would like to increase with 213 or 0.0.255.255 if
> > I want to increase by a /16 at a time?
> 
> More like:
> 
> Initial IPv4 address:	0.0.0.1
> Step:			0.1.0.0
> Mask:			127.0.0.1
> 
> addr = (addr + step) & mask;
> 
> Cycles through all /16's below 128

Which I guess is doable with the patch using these options;
(dst|src)_min 0.0.0.1
(dst|src)_max 128.0.0.0
(dst|src)_step 65535

but you want (dst|src)_step to be specified using dot-quad
instead of an integer?

   Kristian.

-- 
Kristian Larsson                                        KLL-RIPE
+46 704 264511			              kll@spritelink.net

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: pktgen IP address stepping
  2010-12-16 18:02 ` Joe Perches
  2010-12-16 18:30   ` Kristian Larsson
@ 2010-12-16 21:11   ` Kristian Larsson
  2010-12-16 21:22     ` Joe Perches
  1 sibling, 1 reply; 12+ messages in thread
From: Kristian Larsson @ 2010-12-16 21:11 UTC (permalink / raw)
  To: Joe Perches; +Cc: David S. Miller, netdev

On Thu, Dec 16, 2010 at 10:02:12AM -0800, Joe Perches wrote:
> pr_<foo> calls are already prefixed with pktgen via pr_fmt,
> you don't need to add it to the format string.

I can see at least two different cases of debug statements;

    if (debug)
        pr_info("Delay set at: %llu ns\n", pkt_dev->delay);


    if (debug)
        printk(KERN_DEBUG "pktgen: dst_min set to: %s\n",
               pkt_dev->dst_min);


what's the reasoning behind using one or the other?

   -K

-- 
Kristian Larsson                                        KLL-RIPE
+46 704 264511			              kll@spritelink.net

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: pktgen IP address stepping
  2010-12-16 21:11   ` Kristian Larsson
@ 2010-12-16 21:22     ` Joe Perches
  0 siblings, 0 replies; 12+ messages in thread
From: Joe Perches @ 2010-12-16 21:22 UTC (permalink / raw)
  To: Kristian Larsson; +Cc: David S. Miller, netdev

On Thu, 2010-12-16 at 22:11 +0100, Kristian Larsson wrote:
> On Thu, Dec 16, 2010 at 10:02:12AM -0800, Joe Perches wrote:
> > pr_<foo> calls are already prefixed with pktgen via pr_fmt,
> > you don't need to add it to the format string.
> I can see at least two different cases of debug statements;
>     if (debug)
>         pr_info("Delay set at: %llu ns\n", pkt_dev->delay);
>     if (debug)
>         printk(KERN_DEBUG "pktgen: dst_min set to: %s\n",
>                pkt_dev->dst_min);
> what's the reasoning behind using one or the other?

pr_info(fmt, ...)	emits at KERN_INFO level and uses pr_fmt(fmt)
printk(KERN_DEBUG	emits at KERN_DEBUG level and doesn't use pr_fmt
pr_debug(fmt, ...)	does use pr_fmt and is optionally compiled.

I can't say why the author decided to use a info level output
for the delay set message.  Maybe that person doesn't like to see
KERN_DEBUG messages.

cheers, Joe


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next] pktgen: Remove unnecessary prefix from pr_<level>
  2010-12-16 19:02     ` [PATCH net-next] pktgen: Remove unnecessary prefix from pr_<level> Joe Perches
@ 2010-12-20 18:30       ` David Miller
  0 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2010-12-20 18:30 UTC (permalink / raw)
  To: joe; +Cc: kristian, netdev

From: Joe Perches <joe@perches.com>
Date: Thu, 16 Dec 2010 11:02:33 -0800

> Remove "pktgen: " prefix string from one pr_info.
> pr_fmt adds it, so this is a duplicate.
> 
> Signed-off-by: Joe Perches <joe@perches.com>

Applied.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2010-12-20 18:29 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-12-16 17:28 pktgen IP address stepping Kristian Larsson
2010-12-16 17:44 ` David Miller
2010-12-16 18:27   ` Kristian Larsson
2010-12-16 18:34     ` David Miller
2010-12-16 18:02 ` Joe Perches
2010-12-16 18:30   ` Kristian Larsson
2010-12-16 19:02     ` [PATCH net-next] pktgen: Remove unnecessary prefix from pr_<level> Joe Perches
2010-12-20 18:30       ` David Miller
2010-12-16 19:18     ` pktgen IP address stepping Joe Perches
2010-12-16 20:34       ` Kristian Larsson
2010-12-16 21:11   ` Kristian Larsson
2010-12-16 21:22     ` Joe Perches

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.