Skip to contentSkip to conversation

pktgen update for net-next (2.6.32)

PATCH In review Stephen Hemminger 6 participants 28 messages Aug 27, 2009 – Sep 11, 2009

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover pktgen update for net-next (2.6.32)

The biggest change is switching to monotonic clock (ktime) and high resolution timers for the interpacket delay.

#1 lore raw

PATCH 1/14 pktgen: minor cleanup

A couple of minor functions can be written more compactly.

---
 net/core/pktgen.c |   25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)
net/core/pktgen.c +9 -16
@@ -509,7 +509,7 @@ static const struct file_operations pktg
 
 static int pktgen_if_show(struct seq_file *seq, void *v)
 {
-	struct pktgen_dev *pkt_dev = seq->private;
+	const struct pktgen_dev *pkt_dev = seq->private;
 	__u64 sa;
 	__u64 stopped;
 	__u64 now = getCurUs();
@@ -1670,7 +1670,7 @@ static const struct file_operations pktg
 static int pktgen_thread_show(struct seq_file *seq, void *v)
 {
 	struct pktgen_thread *t = seq->private;
-	struct pktgen_dev *pkt_dev;
+	const struct pktgen_dev *pkt_dev;
 
 	BUG_ON(!t);
 
@@ -2120,13 +2120,9 @@ static inline void set_pkt_overhead(stru
 	pkt_dev->pkt_overhead += SVLAN_TAG_SIZE(pkt_dev);
 }
 
-static inline int f_seen(struct pktgen_dev *pkt_dev, int flow)
+static inline int f_seen(const struct pktgen_dev *pkt_dev, int flow)
 {
-
-	if (pkt_dev->flows[flow].flags & F_INIT)
-		return 1;
-	else
-		return 0;
+	return !!(pkt_dev->flows[flow].flags & F_INIT);
 }
 
 static inline int f_pick(struct pktgen_dev *pkt_dev)
@@ -3101,17 +3097,14 @@ static void pktgen_stop_all_threads_ifs(
 	mutex_unlock(&pktgen_thread_lock);
 }
 
-static int thread_is_running(struct pktgen_thread *t)
+static int thread_is_running(const struct pktgen_thread *t)
 {
-	struct pktgen_dev *pkt_dev;
-	int res = 0;
+	const struct pktgen_dev *pkt_dev;
 
 	list_for_each_entry(pkt_dev, &t->if_list, list)
-		if (pkt_dev->running) {
-			res = 1;
-			break;
-		}
-	return res;
+		if (pkt_dev->running)
+			return 1;
+	return 0;
 }
 
 static int pktgen_wait_thread_run(struct pktgen_thread *t)
#2 lore raw

PATCH 2/14 pktgen: change inlining

In reply to Stephen Hemminger

Don't force inlining where not needed. Gcc does better job of deciding to inline local functions.

---
 net/core/pktgen.c |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)
net/core/pktgen.c +7 -8
@@ -2438,7 +2438,7 @@ error:
 	return err;
 }
 
-static inline void free_SAs(struct pktgen_dev *pkt_dev)
+static void free_SAs(struct pktgen_dev *pkt_dev)
 {
 	if (pkt_dev->cflows) {
 		/* let go of the SAs if we have them */
@@ -2453,7 +2453,7 @@ static inline void free_SAs(struct pktge
 	}
 }
 
-static inline int process_ipsec(struct pktgen_dev *pkt_dev,
+static int process_ipsec(struct pktgen_dev *pkt_dev,
 			      struct sk_buff *skb, __be16 protocol)
 {
 	if (pkt_dev->flags & F_IPSEC_ON) {
@@ -3029,8 +3029,8 @@ static struct sk_buff *fill_packet_ipv6(
 	return skb;
 }
 
-static inline struct sk_buff *fill_packet(struct net_device *odev,
-					  struct pktgen_dev *pkt_dev)
+static struct sk_buff *fill_packet(struct net_device *odev,
+				   struct pktgen_dev *pkt_dev)
 {
 	if (pkt_dev->flags & F_IPV6)
 		return fill_packet_ipv6(odev, pkt_dev);
@@ -3341,13 +3341,12 @@ static void pktgen_rem_thread(struct pkt
 	mutex_unlock(&pktgen_thread_lock);
 }
 
-static __inline__ void pktgen_xmit(struct pktgen_dev *pkt_dev)
+static void pktgen_xmit(struct pktgen_dev *pkt_dev)
 {
 	struct net_device *odev = pkt_dev->odev;
 	int (*xmit)(struct sk_buff *, struct net_device *)
 		= odev->netdev_ops->ndo_start_xmit;
 	struct netdev_queue *txq;
-	__u64 idle_start = 0;
 	u16 queue_map;
 	int ret;
 
@@ -3379,7 +3378,7 @@ static __inline__ void pktgen_xmit(struc
 	if (netif_tx_queue_stopped(txq) ||
 	    netif_tx_queue_frozen(txq) ||
 	    need_resched()) {
-		idle_start = getCurUs();
+		u64 idle_start = getCurUs();
 
 		if (!netif_running(odev)) {
 			pktgen_stop_device(pkt_dev);
@@ -3475,7 +3474,7 @@ static __inline__ void pktgen_xmit(struc
 	/* If pkt_dev->count is zero, then run forever */
 	if ((pkt_dev->count != 0) && (pkt_dev->sofar >= pkt_dev->count)) {
 		if (atomic_read(&(pkt_dev->skb->users)) != 1) {
-			idle_start = getCurUs();
+			u64 idle_start = getCurUs();
 			while (atomic_read(&(pkt_dev->skb->users)) != 1) {
 				if (signal_pending(current)) {
 					break;
#3 lore raw

PATCH 3/14 pktgen: mark read-only/mostly variables

In reply to Stephen Hemminger
---
 net/core/pktgen.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
net/core/pktgen.c +5 -5
@@ -414,7 +414,7 @@ static __u64 getCurUs(void)
 
 /* old include end */
 
-static char version[] __initdata = VERSION;
+static const char version[] __initconst = VERSION;
 
 static int pktgen_remove_device(struct pktgen_thread *t, struct pktgen_dev *i);
 static int pktgen_add_device(struct pktgen_thread *t, const char *ifname);
@@ -432,10 +432,10 @@ static unsigned int scan_ip6(const char 
 static unsigned int fmt_ip6(char *s, const char ip[16]);
 
 /* Module parameters, defaults. */
-static int pg_count_d = 1000;	/* 1000 pkts by default */
-static int pg_delay_d;
-static int pg_clone_skb_d;
-static int debug;
+static int pg_count_d __read_mostly = 1000;
+static int pg_delay_d __read_mostly;
+static int pg_clone_skb_d  __read_mostly;
+static int debug  __read_mostly;
 
 static DEFINE_MUTEX(pktgen_thread_lock);
 static LIST_HEAD(pktgen_threads);
#4 lore raw

PATCH 4/14 pktgen: stop_device cleanup

In reply to Stephen Hemminger

All the callers were freeing skb after stopping device. Remove unneeded forward decl.

---
 net/core/pktgen.c |   12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)
net/core/pktgen.c +3 -9
@@ -424,7 +424,7 @@ static int pktgen_device_event(struct no
 static void pktgen_run_all_threads(void);
 static void pktgen_reset_all_threads(void);
 static void pktgen_stop_all_threads_ifs(void);
-static int pktgen_stop_device(struct pktgen_dev *pkt_dev);
+
 static void pktgen_stop(struct pktgen_thread *t);
 static void pktgen_clear_counters(struct pktgen_dev *pkt_dev);
 
@@ -3221,7 +3221,6 @@ static void show_results(struct pktgen_d
 }
 
 /* Set stopped-at timer, remove from running list, do counters & statistics */
-
 static int pktgen_stop_device(struct pktgen_dev *pkt_dev)
 {
 	int nr_frags = pkt_dev->skb ? skb_shinfo(pkt_dev->skb)->nr_frags : -1;
@@ -3232,6 +3231,8 @@ static int pktgen_stop_device(struct pkt
 		return -EINVAL;
 	}
 
+	kfree_skb(pkt_dev->skb);
+	pkt_dev->skb = NULL;
 	pkt_dev->stopped_at = getCurUs();
 	pkt_dev->running = 0;
 
@@ -3268,9 +3269,6 @@ static void pktgen_stop(struct pktgen_th
 
 	list_for_each_entry(pkt_dev, &t->if_list, list) {
 		pktgen_stop_device(pkt_dev);
-		kfree_skb(pkt_dev->skb);
-
-		pkt_dev->skb = NULL;
 	}
 
 	if_unlock(t);
@@ -3382,8 +3380,6 @@ static void pktgen_xmit(struct pktgen_de
 
 		if (!netif_running(odev)) {
 			pktgen_stop_device(pkt_dev);
-			kfree_skb(pkt_dev->skb);
-			pkt_dev->skb = NULL;
 			goto out;
 		}
 		if (need_resched())
@@ -3486,8 +3482,6 @@ static void pktgen_xmit(struct pktgen_de
 
 		/* Done with this */
 		pktgen_stop_device(pkt_dev);
-		kfree_skb(pkt_dev->skb);
-		pkt_dev->skb = NULL;
 	}
 out:;
 }
#5 lore raw

PATCH 5/14 pktgen: xmit logic reorganization

In reply to Stephen Hemminger

Do some reorganization of transmit logic path:
* move transmit queue full idle to separate routine
* add a cpu_relax()
* eliminate some of the uneeded goto's
* if queue is still stopped, go back to main thread loop.
* don't give up transmitting if quantum is exhausted (be greedy)

---
 net/core/pktgen.c |   55 +++++++++++++++++++++++-------------------------------
 1 file changed, 24 insertions(+), 31 deletions(-)
net/core/pktgen.c +24 -31
@@ -3339,6 +3339,18 @@ static void pktgen_rem_thread(struct pkt
 	mutex_unlock(&pktgen_thread_lock);
 }
 
+static void idle(struct pktgen_dev *pkt_dev)
+{
+	u64 idle_start = getCurUs();
+
+	if (need_resched())
+		schedule();
+	else
+		cpu_relax();
+
+	pkt_dev->idle_acc += getCurUs() - idle_start;
+}
+
 static void pktgen_xmit(struct pktgen_dev *pkt_dev)
 {
 	struct net_device *odev = pkt_dev->odev;
@@ -3361,7 +3373,7 @@ static void pktgen_xmit(struct pktgen_de
 		if (pkt_dev->delay_us == 0x7FFFFFFF) {
 			pkt_dev->next_tx_us = getCurUs() + pkt_dev->delay_us;
 			pkt_dev->next_tx_ns = pkt_dev->delay_ns;
-			goto out;
+			return;
 		}
 	}
 
@@ -3373,26 +3385,14 @@ static void pktgen_xmit(struct pktgen_de
 	}
 
 	txq = netdev_get_tx_queue(odev, queue_map);
-	if (netif_tx_queue_stopped(txq) ||
-	    netif_tx_queue_frozen(txq) ||
-	    need_resched()) {
-		u64 idle_start = getCurUs();
-
-		if (!netif_running(odev)) {
+	/* Did we saturate the queue already? */
+	if (netif_tx_queue_stopped(txq) || netif_tx_queue_frozen(txq)) {
+		/* If device is down, then all queues are permnantly frozen */
+		if (netif_running(odev))
+			idle(pkt_dev);
+		else
 			pktgen_stop_device(pkt_dev);
-			goto out;
-		}
-		if (need_resched())
-			schedule();
-
-		pkt_dev->idle_acc += getCurUs() - idle_start;
-
-		if (netif_tx_queue_stopped(txq) ||
-		    netif_tx_queue_frozen(txq)) {
-			pkt_dev->next_tx_us = getCurUs();	/* TODO */
-			pkt_dev->next_tx_ns = 0;
-			goto out;	/* Try the next interface */
-		}
+		return;
 	}
 
 	if (pkt_dev->last_ok || !pkt_dev->skb) {
@@ -3407,7 +3407,7 @@ static void pktgen_xmit(struct pktgen_de
 				       "allocate skb in fill_packet.\n");
 				schedule();
 				pkt_dev->clone_count--;	/* back out increment, OOM */
-				goto out;
+				return;
 			}
 			pkt_dev->allocated_skbs++;
 			pkt_dev->clone_count = 0;	/* reset counter */
@@ -3419,9 +3419,9 @@ static void pktgen_xmit(struct pktgen_de
 	txq = netdev_get_tx_queue(odev, queue_map);
 
 	__netif_tx_lock_bh(txq);
-	if (!netif_tx_queue_stopped(txq) &&
-	    !netif_tx_queue_frozen(txq)) {
-
+	if (unlikely(netif_tx_queue_stopped(txq) || netif_tx_queue_frozen(txq)))
+		pkt_dev->last_ok = 0;
+	else {
 		atomic_inc(&(pkt_dev->skb->users));
 	      retry_now:
 		ret = (*xmit)(pkt_dev->skb, odev);
@@ -3458,13 +3458,6 @@ static void pktgen_xmit(struct pktgen_de
 			pkt_dev->next_tx_ns -= 1000;
 		}
 	}
-
-	else {			/* Retry it next time */
-		pkt_dev->last_ok = 0;
-		pkt_dev->next_tx_us = getCurUs();	/* TODO */
-		pkt_dev->next_tx_ns = 0;
-	}
-
 	__netif_tx_unlock_bh(txq);
 
 	/* If pkt_dev->count is zero, then run forever */
#6 lore raw

PATCH 6/14 pktgen: cleanup clone count test

In reply to Stephen Hemminger

The if statement to test for "should a new packet be used" can be simplified.

---
 net/core/pktgen.c |   32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)
net/core/pktgen.c +15 -17
@@ -3395,23 +3395,22 @@ static void pktgen_xmit(struct pktgen_de
 		return;
 	}
 
-	if (pkt_dev->last_ok || !pkt_dev->skb) {
-		if ((++pkt_dev->clone_count >= pkt_dev->clone_skb)
-		    || (!pkt_dev->skb)) {
-			/* build a new pkt */
-			kfree_skb(pkt_dev->skb);
-
-			pkt_dev->skb = fill_packet(odev, pkt_dev);
-			if (pkt_dev->skb == NULL) {
-				printk(KERN_ERR "pktgen: ERROR: couldn't "
-				       "allocate skb in fill_packet.\n");
-				schedule();
-				pkt_dev->clone_count--;	/* back out increment, OOM */
-				return;
-			}
-			pkt_dev->allocated_skbs++;
-			pkt_dev->clone_count = 0;	/* reset counter */
+	if (!pkt_dev->skb || (pkt_dev->last_ok &&
+			      ++pkt_dev->clone_count >= pkt_dev->clone_skb)) {
+		/* build a new pkt */
+		kfree_skb(pkt_dev->skb);
+
+		pkt_dev->skb = fill_packet(odev, pkt_dev);
+		if (pkt_dev->skb == NULL) {
+			printk(KERN_ERR "pktgen: ERROR: couldn't "
+			       "allocate skb in fill_packet.\n");
+			schedule();
+			pkt_dev->clone_count--;	/* back out increment, OOM */
+			return;
 		}
+
+		pkt_dev->allocated_skbs++;
+		pkt_dev->clone_count = 0;	/* reset counter */
 	}
 
 	/* fill_packet() might have changed the queue */
@@ -3476,7 +3475,6 @@ static void pktgen_xmit(struct pktgen_de
 		/* Done with this */
 		pktgen_stop_device(pkt_dev);
 	}
-out:;
 }
 
 /*
#7 lore raw

PATCH 7/14 pktgen: use netdev_alloc_skb

In reply to Stephen Hemminger

netdev_alloc_skb is NUMA node aware.
Also, don't exhaust atomic emergency pool. Don't want pktgen to cause OOM behaviour.

---
 net/core/pktgen.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
net/core/pktgen.c +6 -4
@@ -2539,8 +2539,9 @@ static struct sk_buff *fill_packet_ipv4(
 	mod_cur_headers(pkt_dev);
 
 	datalen = (odev->hard_header_len + 16) & ~0xf;
-	skb = alloc_skb(pkt_dev->cur_pkt_size + 64 + datalen +
-			pkt_dev->pkt_overhead, GFP_ATOMIC);
+	skb = __netdev_alloc_skb(odev,
+				 pkt_dev->cur_pkt_size + 64
+				 + datalen + pkt_dev->pkt_overhead, GFP_NOWAIT);
 	if (!skb) {
 		sprintf(pkt_dev->result, "No memory");
 		return NULL;
@@ -2878,8 +2879,9 @@ static struct sk_buff *fill_packet_ipv6(
 	queue_map = pkt_dev->cur_queue_map;
 	mod_cur_headers(pkt_dev);
 
-	skb = alloc_skb(pkt_dev->cur_pkt_size + 64 + 16 +
-			pkt_dev->pkt_overhead, GFP_ATOMIC);
+	skb = __netdev_alloc_skb(odev,
+				 pkt_dev->cur_pkt_size + 64
+				 + 16 + pkt_dev->pkt_overhead, GFP_NOWAIT);
 	if (!skb) {
 		sprintf(pkt_dev->result, "No memory");
 		return NULL;
#8 lore raw

PATCH 8/14 pktgen: reorganize transmit loop

In reply to Stephen Hemminger

Handle standard (and non-standard) return values in a switch.

---
 net/core/pktgen.c |   26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)
net/core/pktgen.c +14 -12
@@ -3424,27 +3424,29 @@ static void pktgen_xmit(struct pktgen_de
 		pkt_dev->last_ok = 0;
 	else {
 		atomic_inc(&(pkt_dev->skb->users));
-	      retry_now:
+
+	retry_now:
 		ret = (*xmit)(pkt_dev->skb, odev);
-		if (likely(ret == NETDEV_TX_OK)) {
+		switch (ret) {
+		case NETDEV_TX_OK:
 			txq_trans_update(txq);
 			pkt_dev->last_ok = 1;
 			pkt_dev->sofar++;
 			pkt_dev->seq_num++;
 			pkt_dev->tx_bytes += pkt_dev->cur_pkt_size;
-
-		} else if (ret == NETDEV_TX_LOCKED
-			   && (odev->features & NETIF_F_LLTX)) {
+			break;
+		case NETDEV_TX_LOCKED:
 			cpu_relax();
 			goto retry_now;
-		} else {	/* Retry it next time */
-
-			atomic_dec(&(pkt_dev->skb->users));
-
-			if (debug && net_ratelimit())
-				printk(KERN_INFO "pktgen: Hard xmit error\n");
-
+		default: /* Drivers are not supposed to return other values! */
+			if (net_ratelimit())
+				pr_info("pktgen: %s xmit error: %d\n",
+					odev->name, ret);
 			pkt_dev->errors++;
+			/* fallthru */
+		case NETDEV_TX_BUSY:
+			/* Retry it next time */
+			atomic_dec(&(pkt_dev->skb->users));
 			pkt_dev->last_ok = 0;
 		}
 
#9 lore raw

Re: [PATCH 08/14] pktgen: reorganize transmit loop

+ default: /* Drivers are not supposed to return other values! */
+ if (net_ratelimit())
+ pr_info("pktgen: %s xmit error: %d\n",

+					odev->name, ret);
			pkt_dev->errors++;

I believe this is faulty. Things like vlans can send pkts to qdiscs of the underlying device and those can return other values.

Patric McHardy put in some patches recently to achieve this in a more uniform manner:

http://patchwork.ozlabs.org/patch/28340/

Thanks,
Ben

Signature · 1 line
Ben Greear <[email protected]> Candela Technologies Inc  http://www.candelatech.com
#10 lore raw

Re: [PATCH 08/14] pktgen: reorganize transmit loop

Quoted 11 lines · On Thu, 27 Aug 2009 20:52:32 -0700 Ben Greear <[email protected]> wrote:
(quoted diff) +1 -0
+		default: /* Drivers are not supposed to return other

values! */

(quoted diff) +2 -0
+			if (net_ratelimit())
+				pr_info("pktgen: %s xmit error:

%d\n",

net/core/pktgen.c +1 -0
+					odev->name, ret);
 			pkt_dev->errors++;

I believe this is faulty. Things like vlans can send pkts to qdiscs of the underlying device and those can return other values.

Quoted 8 lines

Patric McHardy put in some patches recently to achieve this in a more uniform manner:

http://patchwork.ozlabs.org/patch/28340/

Thanks,
Ben

Since pktgen has its own way of generating vlan tags, it makes no sense to use it on top of 8021q vlan driver.

#11 lore raw

Re: [PATCH 08/14] pktgen: reorganize transmit loop

Quoted 17 lines · Stephen Hemminger
Quoted 14 lines · On Thu, 27 Aug 2009 20:52:32 -0700 Ben Greear<[email protected]> wrote:
(quoted diff) +1 -0
+		default: /* Drivers are not supposed to return other

values! */

(quoted diff) +2 -0
+			if (net_ratelimit())
+				pr_info("pktgen: %s xmit error:

%d\n",

net/core/pktgen.c +1 -0
+					odev->name, ret);
  			pkt_dev->errors++;

I believe this is faulty. Things like vlans can send pkts to qdiscs of the underlying device and those can return other values.

Patric McHardy put in some patches recently to achieve this in a more uniform manner:

Quoted 8 lines
Quoted 5 lines

http://patchwork.ozlabs.org/patch/28340/

Thanks,
Ben

Since pktgen has its own way of generating vlan tags, it makes no sense to use it on top of 8021q vlan driver.

Maybe someone wants to test their virtual device driver using pktgen?

Or generate traffic with lots of different vlan IDs & associated IP, mac-addresses, etc?

Same issue applies to mac-vlans and possibly other virtual devices.

Thanks,
Ben

Signature · 2 lines
Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com
maintainer
#12 lore raw

Re: [PATCH 08/14] pktgen: reorganize transmit loop

In reply to Stephen Hemminger

From: Stephen Hemminger <[email protected]>
Date: Thu, 27 Aug 2009 22:49:02 -0700

Quoted 17 lines
Quoted 14 lines · On Thu, 27 Aug 2009 20:52:32 -0700 Ben Greear <[email protected]> wrote:
(quoted diff) +1 -0
+		default: /* Drivers are not supposed to return other

values! */

(quoted diff) +2 -0
+			if (net_ratelimit())
+				pr_info("pktgen: %s xmit error:

%d\n",

net/core/pktgen.c +1 -0
+					odev->name, ret);
 			pkt_dev->errors++;

I believe this is faulty. Things like vlans can send pkts to qdiscs of the underlying device and those can return other values.

Patric McHardy put in some patches recently to achieve this in a more uniform manner:

Quoted 8 lines
Quoted 5 lines

http://patchwork.ozlabs.org/patch/28340/

Thanks,
Ben

Since pktgen has its own way of generating vlan tags, it makes no sense to use it on top of 8021q vlan driver.

I think Patrick's goals are quite sound, and with his patch we could let pktgen transmit over vlan just like any other device.

Otherwise we give no way to use pktgen to test the VLAN transmit path.

#13 lore raw

Re: [PATCH 08/14] pktgen: reorganize transmit loop

Quoted 29 lines · On Fri, 28 Aug 2009 23:04:28 -0700 (PDT) David Miller <[email protected]>…

From: Stephen Hemminger <[email protected]>
Date: Thu, 27 Aug 2009 22:49:02 -0700

Quoted 17 lines
Quoted 14 lines · On Thu, 27 Aug 2009 20:52:32 -0700 Ben Greear <[email protected]> wrote:
(quoted diff) +1 -0
+		default: /* Drivers are not supposed to return other

values! */

(quoted diff) +2 -0
+			if (net_ratelimit())
+				pr_info("pktgen: %s xmit error:

%d\n",

net/core/pktgen.c +1 -0
+					odev->name, ret);
 			pkt_dev->errors++;

I believe this is faulty. Things like vlans can send pkts to qdiscs of the underlying device and those can return other values.

Patric McHardy put in some patches recently to achieve this in a more uniform manner:

Quoted 8 lines
Quoted 5 lines

http://patchwork.ozlabs.org/patch/28340/

Thanks,
Ben

Since pktgen has its own way of generating vlan tags, it makes no sense to use it on top of 8021q vlan driver.

Quoted 4 lines

I think Patrick's goals are quite sound, and with his patch we could let pktgen transmit over vlan just like any other device.

Otherwise we give no way to use pktgen to test the VLAN transmit path.

The only valid returns from device are OK, BUSY, or LOCKED anything else is an error and should never occur.

If the vlan code returns other values, it must translate.

#14 lore raw

Re: [PATCH 08/14] pktgen: reorganize transmit loop

Quoted 6 lines · Stephen Hemminger
Quoted 3 lines · On Fri, 28 Aug 2009 23:04:28 -0700 (PDT) David Miller <[email protected]>…

I think Patrick's goals are quite sound, and with his patch we could let pktgen transmit over vlan just like any other device.

Quoted 6 lines
Quoted 1 line

Otherwise we give no way to use pktgen to test the VLAN transmit path.

The only valid returns from device are OK, BUSY, or LOCKED anything else is an error and should never occur.

If the vlan code returns other values, it must translate.

That was part of my patches to allow hard_start_xmit() to return NETDEV_TX codes, NET_XMIT codes to propagate transmission qdisc state upwards through virtual devices, as well as errno codes to propagate errors from virtual devices, like EHOSTUNREACH from an IP tunnel device.

I'll refresh that patch and will post it tonight or tommorrow.

#15 lore raw

PATCH 9/14 pktgen: avoid calling gettimeofday

In reply to Stephen Hemminger

If not using delay then no need to update next_tx after each packet sent. This allows pktgen to send faster especially on systems with slower clock sources.

---
 net/core/pktgen.c |   20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)
net/core/pktgen.c +11 -9
@@ -3450,15 +3450,17 @@ static void pktgen_xmit(struct pktgen_de
 			pkt_dev->last_ok = 0;
 		}
 
-		pkt_dev->next_tx_us = getCurUs();
-		pkt_dev->next_tx_ns = 0;
-
-		pkt_dev->next_tx_us += pkt_dev->delay_us;
-		pkt_dev->next_tx_ns += pkt_dev->delay_ns;
-
-		if (pkt_dev->next_tx_ns > 1000) {
-			pkt_dev->next_tx_us++;
-			pkt_dev->next_tx_ns -= 1000;
+		if (pkt_dev->delay_us || pkt_dev->delay_ns) {
+			pkt_dev->next_tx_us = getCurUs();
+			pkt_dev->next_tx_ns = 0;
+
+			pkt_dev->next_tx_us += pkt_dev->delay_us;
+			pkt_dev->next_tx_ns += pkt_dev->delay_ns;
+
+			if (pkt_dev->next_tx_ns > 1000) {
+				pkt_dev->next_tx_us++;
+				pkt_dev->next_tx_ns -= 1000;
+			}
 		}
 	}
 	__netif_tx_unlock_bh(txq);
#16 lore raw

PATCH 10/14 pktgen: convert to use ktime_t

In reply to Stephen Hemminger

The kernel ktime_t is a nice generic infrastructure for mananging high resolution times, as is done in pktgen.

---
 net/core/pktgen.c |  185 ++++++++++++++++++++++++------------------------------
 1 file changed, 85 insertions(+), 100 deletions(-)
net/core/pktgen.c +84 -100
@@ -246,16 +246,14 @@ struct pktgen_dev {
 	int max_pkt_size;	/* = ETH_ZLEN; */
 	int pkt_overhead;	/* overhead for MPLS, VLANs, IPSEC etc */
 	int nfrags;
-	__u32 delay_us;		/* Default delay */
-	__u32 delay_ns;
+	u64 delay;		/* nano-seconds */
+
 	__u64 count;		/* Default No packets to send */
 	__u64 sofar;		/* How many pkts we've sent so far */
 	__u64 tx_bytes;		/* How many bytes we've transmitted */
 	__u64 errors;		/* Errors when trying to transmit, pkts will be re-sent */
 
 	/* runtime counters relating to clone_skb */
-	__u64 next_tx_us;	/* timestamp of when to tx next */
-	__u32 next_tx_ns;
 
 	__u64 allocated_skbs;
 	__u32 clone_count;
@@ -263,9 +261,11 @@ struct pktgen_dev {
 				 * Or a failed transmit of some sort?  This will keep
 				 * sequence numbers in order, for example.
 				 */
-	__u64 started_at;	/* micro-seconds */
-	__u64 stopped_at;	/* micro-seconds */
-	__u64 idle_acc;		/* micro-seconds */
+	ktime_t next_tx;
+	ktime_t started_at;
+	ktime_t stopped_at;
+	u64	idle_acc;	/* nano-seconds */
+
 	__u32 seq_num;
 
 	int clone_skb;		/* Use multiple SKBs during packet gen.  If this number
@@ -397,23 +397,20 @@ struct pktgen_thread {
 #define REMOVE 1
 #define FIND   0
 
-/** Convert to micro-seconds */
-static inline __u64 tv_to_us(const struct timeval *tv)
+static inline ktime_t ktime_now(void)
 {
-	__u64 us = tv->tv_usec;
-	us += (__u64) tv->tv_sec * (__u64) 1000000;
-	return us;
+	struct timespec ts;
+	ktime_get_ts(&ts);
+
+	return timespec_to_ktime(ts);
 }
 
-static __u64 getCurUs(void)
+/* This works even if 32 bit because of careful byte order choice */
+static inline int ktime_lt(const ktime_t cmp1, const ktime_t cmp2)
 {
-	struct timeval tv;
-	do_gettimeofday(&tv);
-	return tv_to_us(&tv);
+	return cmp1.tv64 < cmp2.tv64;
 }
 
-/* old include end */
-
 static const char version[] __initconst = VERSION;
 
 static int pktgen_remove_device(struct pktgen_thread *t, struct pktgen_dev *i);
@@ -510,9 +507,8 @@ static const struct file_operations pktg
 static int pktgen_if_show(struct seq_file *seq, void *v)
 {
 	const struct pktgen_dev *pkt_dev = seq->private;
-	__u64 sa;
-	__u64 stopped;
-	__u64 now = getCurUs();
+	ktime_t stopped;
+	u64 idle;
 
 	seq_printf(seq,
 		   "Params: count %llu  min_pkt_size: %u  max_pkt_size: %u\n",
@@ -520,9 +516,8 @@ static int pktgen_if_show(struct seq_fil
 		   pkt_dev->max_pkt_size);
 
 	seq_printf(seq,
-		   "     frags: %d  delay: %u  clone_skb: %d  ifname: %s\n",
-		   pkt_dev->nfrags,
-		   1000 * pkt_dev->delay_us + pkt_dev->delay_ns,
+		   "     frags: %d  delay: %llu  clone_skb: %d  ifname: %s\n",
+		   pkt_dev->nfrags, (unsigned long long) pkt_dev->delay,
 		   pkt_dev->clone_skb, pkt_dev->odev->name);
 
 	seq_printf(seq, "     flows: %u flowlen: %u\n", pkt_dev->cflows,
@@ -654,17 +649,21 @@ static int pktgen_if_show(struct seq_fil
 
 	seq_puts(seq, "\n");
 
-	sa = pkt_dev->started_at;
-	stopped = pkt_dev->stopped_at;
-	if (pkt_dev->running)
-		stopped = now;	/* not really stopped, more like last-running-at */
+	/* not really stopped, more like last-running-at */
+	stopped = pkt_dev->running ? ktime_now() : pkt_dev->stopped_at;
+	idle = pkt_dev->idle_acc;
+	do_div(idle, NSEC_PER_USEC);
 
 	seq_printf(seq,
-		   "Current:\n     pkts-sofar: %llu  errors: %llu\n     started: %lluus  stopped: %lluus idle: %lluus\n",
+		   "Current:\n     pkts-sofar: %llu  errors: %llu\n",
 		   (unsigned long long)pkt_dev->sofar,
-		   (unsigned long long)pkt_dev->errors, (unsigned long long)sa,
-		   (unsigned long long)stopped,
-		   (unsigned long long)pkt_dev->idle_acc);
+		   (unsigned long long)pkt_dev->errors);
+
+	seq_printf(seq,
+		   "     started: %lluus  stopped: %lluus idle: %lluus\n",
+		   (unsigned long long) ktime_to_us(pkt_dev->started_at),
+		   (unsigned long long) ktime_to_us(stopped),
+		   (unsigned long long) idle);
 
 	seq_printf(seq,
 		   "     seq_num: %d  cur_dst_mac_offset: %d  cur_src_mac_offset: %d\n",
@@ -950,15 +949,13 @@ static ssize_t pktgen_if_write(struct fi
 			return len;
 		}
 		i += len;
-		if (value == 0x7FFFFFFF) {
-			pkt_dev->delay_us = 0x7FFFFFFF;
-			pkt_dev->delay_ns = 0;
-		} else {
-			pkt_dev->delay_us = value / 1000;
-			pkt_dev->delay_ns = value % 1000;
-		}
-		sprintf(pg_result, "OK: delay=%u",
-			1000 * pkt_dev->delay_us + pkt_dev->delay_ns);
+		if (value == 0x7FFFFFFF)
+			pkt_dev->delay = ULLONG_MAX;
+		else
+			pkt_dev->delay = (u64)value * NSEC_PER_USEC;
+
+		sprintf(pg_result, "OK: delay=%llu",
+			(unsigned long long) pkt_dev->delay);
 		return count;
 	}
 	if (!strcmp(name, "udp_src_min")) {
@@ -2089,27 +2086,33 @@ static void pktgen_setup_inject(struct p
 	pkt_dev->nflows = 0;
 }
 
-static void spin(struct pktgen_dev *pkt_dev, __u64 spin_until_us)
+static inline s64 delta_ns(ktime_t a, ktime_t b)
+{
+	return ktime_to_ns(ktime_sub(a, b));
+}
+
+static void spin(struct pktgen_dev *pkt_dev, ktime_t spin_until)
 {
-	__u64 start;
-	__u64 now;
+	ktime_t start, now;
+	s64 dt;
 
-	start = now = getCurUs();
-	while (now < spin_until_us) {
+	start = now = ktime_now();
+
+	while ((dt = delta_ns(spin_until, now)) > 0) {
 		/* TODO: optimize sleeping behavior */
-		if (spin_until_us - now > jiffies_to_usecs(1) + 1)
+		if (dt > TICK_NSEC)
 			schedule_timeout_interruptible(1);
-		else if (spin_until_us - now > 100) {
+		else if (dt > 100*NSEC_PER_USEC) {
 			if (!pkt_dev->running)
 				return;
 			if (need_resched())
 				schedule();
 		}
 
-		now = getCurUs();
+		now = ktime_now();
 	}
 
-	pkt_dev->idle_acc += now - start;
+	pkt_dev->idle_acc += ktime_to_ns(ktime_sub(now, start));
 }
 
 static inline void set_pkt_overhead(struct pktgen_dev *pkt_dev)
@@ -3070,9 +3073,9 @@ static void pktgen_run(struct pktgen_thr
 			pktgen_clear_counters(pkt_dev);
 			pkt_dev->running = 1;	/* Cranke yeself! */
 			pkt_dev->skb = NULL;
-			pkt_dev->started_at = getCurUs();
-			pkt_dev->next_tx_us = getCurUs();	/* Transmit immediately */
-			pkt_dev->next_tx_ns = 0;
+			pkt_dev->started_at =
+				pkt_dev->next_tx = ktime_now();
+
 			set_pkt_overhead(pkt_dev);
 
 			strcpy(pkt_dev->result, "Starting");
@@ -3188,28 +3191,21 @@ static void pktgen_reset_all_threads(voi
 
 static void show_results(struct pktgen_dev *pkt_dev, int nr_frags)
 {
-	__u64 total_us, bps, mbps, pps, idle;
+	__u64 bps, mbps, pps;
 	char *p = pkt_dev->result;
-
-	total_us = pkt_dev->stopped_at - pkt_dev->started_at;
-
-	idle = pkt_dev->idle_acc;
-
-	p += sprintf(p, "OK: %llu(c%llu+d%llu) usec, %llu (%dbyte,%dfrags)\n",
-		     (unsigned long long)total_us,
-		     (unsigned long long)(total_us - idle),
-		     (unsigned long long)idle,
+	ktime_t elapsed = ktime_sub(pkt_dev->stopped_at,
+				    pkt_dev->started_at);
+	ktime_t idle = ns_to_ktime(pkt_dev->idle_acc);
+
+	p += sprintf(p, "OK: %llu(c%llu+d%llu) nsec, %llu (%dbyte,%dfrags)\n",
+		     (unsigned long long)ktime_to_us(elapsed),
+		     (unsigned long long)ktime_to_us(ktime_sub(elapsed, idle)),
+		     (unsigned long long)ktime_to_us(idle),
 		     (unsigned long long)pkt_dev->sofar,
 		     pkt_dev->cur_pkt_size, nr_frags);
 
-	pps = pkt_dev->sofar * USEC_PER_SEC;
-
-	while ((total_us >> 32) != 0) {
-		pps >>= 1;
-		total_us >>= 1;
-	}
-
-	do_div(pps, total_us);
+	pps = div64_u64(pkt_dev->sofar * NSEC_PER_SEC,
+			ktime_to_ns(elapsed));
 
 	bps = pps * 8 * pkt_dev->cur_pkt_size;
 
@@ -3235,7 +3231,7 @@ static int pktgen_stop_device(struct pkt
 
 	kfree_skb(pkt_dev->skb);
 	pkt_dev->skb = NULL;
-	pkt_dev->stopped_at = getCurUs();
+	pkt_dev->stopped_at = ktime_now();
 	pkt_dev->running = 0;
 
 	show_results(pkt_dev, nr_frags);
@@ -3254,7 +3250,7 @@ static struct pktgen_dev *next_to_run(st
 			continue;
 		if (best == NULL)
 			best = pkt_dev;
-		else if (pkt_dev->next_tx_us < best->next_tx_us)
+		else if (ktime_lt(pkt_dev->next_tx, best->next_tx))
 			best = pkt_dev;
 	}
 	if_unlock(t);
@@ -3343,16 +3339,17 @@ static void pktgen_rem_thread(struct pkt
 
 static void idle(struct pktgen_dev *pkt_dev)
 {
-	u64 idle_start = getCurUs();
+	ktime_t idle_start = ktime_now();
 
 	if (need_resched())
 		schedule();
 	else
 		cpu_relax();
 
-	pkt_dev->idle_acc += getCurUs() - idle_start;
+	pkt_dev->idle_acc += ktime_to_ns(ktime_sub(ktime_now(), idle_start));
 }
 
+
 static void pktgen_xmit(struct pktgen_dev *pkt_dev)
 {
 	struct net_device *odev = pkt_dev->odev;
@@ -3362,19 +3359,15 @@ static void pktgen_xmit(struct pktgen_de
 	u16 queue_map;
 	int ret;
 
-	if (pkt_dev->delay_us || pkt_dev->delay_ns) {
-		u64 now;
-
-		now = getCurUs();
-		if (now < pkt_dev->next_tx_us)
-			spin(pkt_dev, pkt_dev->next_tx_us);
+	if (pkt_dev->delay) {
+		if (ktime_lt(ktime_now(), pkt_dev->next_tx))
+			spin(pkt_dev, pkt_dev->next_tx);
 
 		/* This is max DELAY, this has special meaning of
 		 * "never transmit"
 		 */
-		if (pkt_dev->delay_us == 0x7FFFFFFF) {
-			pkt_dev->next_tx_us = getCurUs() + pkt_dev->delay_us;
-			pkt_dev->next_tx_ns = pkt_dev->delay_ns;
+		if (pkt_dev->delay == ULLONG_MAX) {
+			pkt_dev->next_tx = ktime_add_ns(ktime_now(), ULONG_MAX);
 			return;
 		}
 	}
@@ -3450,32 +3443,24 @@ static void pktgen_xmit(struct pktgen_de
 			pkt_dev->last_ok = 0;
 		}
 
-		if (pkt_dev->delay_us || pkt_dev->delay_ns) {
-			pkt_dev->next_tx_us = getCurUs();
-			pkt_dev->next_tx_ns = 0;
-
-			pkt_dev->next_tx_us += pkt_dev->delay_us;
-			pkt_dev->next_tx_ns += pkt_dev->delay_ns;
-
-			if (pkt_dev->next_tx_ns > 1000) {
-				pkt_dev->next_tx_us++;
-				pkt_dev->next_tx_ns -= 1000;
-			}
-		}
+		if (pkt_dev->delay)
+			pkt_dev->next_tx = ktime_add_ns(ktime_now(),
+							pkt_dev->delay);
 	}
 	__netif_tx_unlock_bh(txq);
 
 	/* If pkt_dev->count is zero, then run forever */
 	if ((pkt_dev->count != 0) && (pkt_dev->sofar >= pkt_dev->count)) {
 		if (atomic_read(&(pkt_dev->skb->users)) != 1) {
-			u64 idle_start = getCurUs();
+			ktime_t idle_start = ktime_now();
 			while (atomic_read(&(pkt_dev->skb->users)) != 1) {
 				if (signal_pending(current)) {
 					break;
 				}
 				schedule();
 			}
-			pkt_dev->idle_acc += getCurUs() - idle_start;
+			pkt_dev->idle_acc += ktime_to_ns(ktime_sub(ktime_now(),
+								   idle_start));
 		}
 
 		/* Done with this */
@@ -3634,8 +3619,7 @@ static int pktgen_add_device(struct pktg
 	pkt_dev->max_pkt_size = ETH_ZLEN;
 	pkt_dev->nfrags = 0;
 	pkt_dev->clone_skb = pg_clone_skb_d;
-	pkt_dev->delay_us = pg_delay_d / 1000;
-	pkt_dev->delay_ns = pg_delay_d % 1000;
+	pkt_dev->delay = pg_delay_d;
 	pkt_dev->count = pg_count_d;
 	pkt_dev->sofar = 0;
 	pkt_dev->udp_src_min = 9;	/* sink port */
#17 lore raw

PATCH 11/14 pktgen: spin using hrtimer

In reply to Stephen Hemminger

This changes how the pktgen thread spins/waits between packets if delay is configured. It uses a high res timer to wait for time to arrive.

---
 kernel/hrtimer.c  |    1 +
 net/core/pktgen.c |   49 ++++++++++++++++++++++++++++---------------------
 2 files changed, 29 insertions(+), 21 deletions(-)
net/core/pktgen.c +28 -21
@@ -131,6 +131,7 @@
 #include <linux/ioport.h>
 #include <linux/interrupt.h>
 #include <linux/capability.h>
+#include <linux/hrtimer.h>
 #include <linux/freezer.h>
 #include <linux/delay.h>
 #include <linux/timer.h>
@@ -2086,33 +2087,40 @@ static void pktgen_setup_inject(struct p
 	pkt_dev->nflows = 0;
 }
 
-static inline s64 delta_ns(ktime_t a, ktime_t b)
-{
-	return ktime_to_ns(ktime_sub(a, b));
-}
 
 static void spin(struct pktgen_dev *pkt_dev, ktime_t spin_until)
 {
-	ktime_t start, now;
-	s64 dt;
+	ktime_t start;
+	s32 remaining;
+	struct hrtimer_sleeper t;
 
-	start = now = ktime_now();
+	hrtimer_init_on_stack(&t.timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
+	hrtimer_set_expires(&t.timer, spin_until);
 
-	while ((dt = delta_ns(spin_until, now)) > 0) {
-		/* TODO: optimize sleeping behavior */
-		if (dt > TICK_NSEC)
-			schedule_timeout_interruptible(1);
-		else if (dt > 100*NSEC_PER_USEC) {
-			if (!pkt_dev->running)
-				return;
-			if (need_resched())
+	remaining = ktime_to_us(hrtimer_expires_remaining(&t.timer));
+	if (remaining <= 0)
+		return;
+
+	start = ktime_now();
+	if (remaining < 100)
+		udelay(remaining); 	/* really small just spin */
+	else {
+		/* see do_nanosleep */
+		hrtimer_init_sleeper(&t, current);
+		do {
+			set_current_state(TASK_INTERRUPTIBLE);
+			hrtimer_start_expires(&t.timer, HRTIMER_MODE_ABS);
+			if (!hrtimer_active(&t.timer))
+				t.task = NULL;
+
+			if (likely(t.task))
 				schedule();
-		}
 
-		now = ktime_now();
+			hrtimer_cancel(&t.timer);
+		} while (t.task && pkt_dev->running && !signal_pending(current));
+		__set_current_state(TASK_RUNNING);
 	}
-
-	pkt_dev->idle_acc += ktime_to_ns(ktime_sub(now, start));
+	pkt_dev->idle_acc += ktime_to_ns(ktime_sub(ktime_now(), start));
 }
 
 static inline void set_pkt_overhead(struct pktgen_dev *pkt_dev)
@@ -3360,8 +3368,7 @@ static void pktgen_xmit(struct pktgen_de
 	int ret;
 
 	if (pkt_dev->delay) {
-		if (ktime_lt(ktime_now(), pkt_dev->next_tx))
-			spin(pkt_dev, pkt_dev->next_tx);
+		spin(pkt_dev, pkt_dev->next_tx);
 
 		/* This is max DELAY, this has special meaning of
 		 * "never transmit"
kernel/hrtimer.c +1 -0
@@ -1477,6 +1477,7 @@ void hrtimer_init_sleeper(struct hrtimer
 	sl->timer.function = hrtimer_wakeup;
 	sl->task = task;
 }
+EXPORT_SYMBOL_GPL(hrtimer_init_sleeper);
 
 static int __sched do_nanosleep(struct hrtimer_sleeper *t, enum hrtimer_mode mode)
 {
#18 lore raw

PATCH 12/14 pktgen: use common idle routine

In reply to Stephen Hemminger

Simpler to have one place that spins and accounts for delays, this will also make the last packet be detected faster for more repeatable timing.

net/core/pktgen.c +4 -10
@@ -3458,16 +3458,10 @@ static void pktgen_xmit(struct pktgen_de
 
 	/* If pkt_dev->count is zero, then run forever */
 	if ((pkt_dev->count != 0) && (pkt_dev->sofar >= pkt_dev->count)) {
-		if (atomic_read(&(pkt_dev->skb->users)) != 1) {
-			ktime_t idle_start = ktime_now();
-			while (atomic_read(&(pkt_dev->skb->users)) != 1) {
-				if (signal_pending(current)) {
-					break;
-				}
-				schedule();
-			}
-			pkt_dev->idle_acc += ktime_to_ns(ktime_sub(ktime_now(),
-								   idle_start));
+		while (atomic_read(&(pkt_dev->skb->users)) != 1) {
+			if (signal_pending(current))
+				break;
+			idle(pkt_dev);
 		}
 
 		/* Done with this */
#19 lore raw

PATCH 13/14 pktgen: cleanup checkpatch warnings

In reply to Stephen Hemminger
net/core/pktgen.c +159 -139
@@ -163,13 +163,14 @@
 #include <asm/byteorder.h>
 #include <linux/rcupdate.h>
 #include <linux/bitops.h>
-#include <asm/io.h>
+#include <linux/io.h>
+#include <linux/timex.h>
+#include <linux/uaccess.h>
 #include <asm/dma.h>
-#include <asm/uaccess.h>
 #include <asm/div64.h>		/* do_div */
-#include <asm/timex.h>
 
-#define VERSION  "pktgen v2.70: Packet Generator for packet performance testing.\n"
+#define VERSION \
+	"pktgen v2.70: Packet Generator for packet performance testing.\n"
 
 #define IP_NAME_SZ 32
 #define MAX_MPLS_LABELS 16 /* This is the max label stack depth */
@@ -207,7 +208,7 @@
 #define PKTGEN_MAGIC 0xbe9be955
 #define PG_PROC_DIR "pktgen"
 #define PGCTRL	    "pgctrl"
-static struct proc_dir_entry *pg_proc_dir = NULL;
+static struct proc_dir_entry *pg_proc_dir;
 
 #define MAX_CFLOWS  65536
 
@@ -232,9 +233,9 @@ struct pktgen_dev {
 	 */
 	struct proc_dir_entry *entry;	/* proc file */
 	struct pktgen_thread *pg_thread;/* the owner */
-	struct list_head list;		/* Used for chaining in the thread's run-queue */
+	struct list_head list;		/* chaining in the thread's run-queue */
 
-	int running;		/* if this changes to false, the test will stop */
+	int running;		/* if false, the test will stop */
 
 	/* If min != max, then we will either do a linear iteration, or
 	 * we will do a random selection from within the range.
@@ -252,15 +253,16 @@ struct pktgen_dev {
 	__u64 count;		/* Default No packets to send */
 	__u64 sofar;		/* How many pkts we've sent so far */
 	__u64 tx_bytes;		/* How many bytes we've transmitted */
-	__u64 errors;		/* Errors when trying to transmit, pkts will be re-sent */
+	__u64 errors;		/* Errors when trying to transmit,
+				   pkts will be re-sent */
 
 	/* runtime counters relating to clone_skb */
 
 	__u64 allocated_skbs;
 	__u32 clone_count;
 	int last_ok;		/* Was last skb sent?
-				 * Or a failed transmit of some sort?  This will keep
-				 * sequence numbers in order, for example.
+				 * Or a failed transmit of some sort?
+				 * This will keep sequence numbers in order
 				 */
 	ktime_t next_tx;
 	ktime_t started_at;
@@ -269,11 +271,14 @@ struct pktgen_dev {
 
 	__u32 seq_num;
 
-	int clone_skb;		/* Use multiple SKBs during packet gen.  If this number
-				 * is greater than 1, then that many copies of the same
-				 * packet will be sent before a new packet is allocated.
-				 * For instance, if you want to send 1024 identical packets
-				 * before creating a new packet, set clone_skb to 1024.
+	int clone_skb;		/*
+				 * Use multiple SKBs during packet gen.
+				 * If this number is greater than 1, then
+				 * that many copies of the same packet will be
+				 * sent before a new packet is allocated.
+				 * If you want to send 1024 identical packets
+				 * before creating a new packet,
+				 * set clone_skb to 1024.
 				 */
 
 	char dst_min[IP_NAME_SZ];	/* IP, ie 1.2.3.4 */
@@ -305,8 +310,10 @@ struct pktgen_dev {
 	__u16 udp_dst_max;	/* exclusive, dest UDP port */
 
 	/* DSCP + ECN */
-	__u8 tos;            /* six most significant bits of (former) IPv4 TOS are for dscp codepoint */
-	__u8 traffic_class;  /* ditto for the (former) Traffic Class in IPv6 (see RFC 3260, sec. 4) */
+	__u8 tos;            /* six MSB of (former) IPv4 TOS
+				are for dscp codepoint */
+	__u8 traffic_class;  /* ditto for the (former) Traffic Class in IPv6
+				(see RFC 3260, sec. 4) */
 
 	/* MPLS */
 	unsigned nr_labels;	/* Depth of stack, 0 = no MPLS */
@@ -347,15 +354,17 @@ struct pktgen_dev {
 	 */
 	__u16 pad;		/* pad out the hh struct to an even 16 bytes */
 
-	struct sk_buff *skb;	/* skb we are to transmit next, mainly used for when we
+	struct sk_buff *skb;	/* skb we are to transmit next, used for when we
 				 * are transmitting the same one multiple times
 				 */
-	struct net_device *odev;	/* The out-going device.  Note that the device should
-					 * have it's pg_info pointer pointing back to this
-					 * device.  This will be set when the user specifies
-					 * the out-going device name (not when the inject is
-					 * started as it used to do.)
-					 */
+	struct net_device *odev; /* The out-going device.
+				  * Note that the device should have it's
+				  * pg_info pointer pointing back to this
+				  * device.
+				  * Set when the user specifies the out-going
+				  * device name (not when the inject is
+				  * started as it used to do.)
+				  */
 	struct flow_state *flows;
 	unsigned cflows;	/* Concurrent flows (config) */
 	unsigned lflow;		/* Flow length  (config) */
@@ -380,13 +389,14 @@ struct pktgen_hdr {
 };
 
 struct pktgen_thread {
-	spinlock_t if_lock;
+	spinlock_t if_lock;		/* for list of devices */
 	struct list_head if_list;	/* All device here */
 	struct list_head th_list;
 	struct task_struct *tsk;
 	char result[512];
 
-	/* Field for thread to receive "posted" events terminate, stop ifs etc. */
+	/* Field for thread to receive "posted" events terminate,
+	   stop ifs etc. */
 
 	u32 control;
 	int cpu;
@@ -453,8 +463,8 @@ static int pgctrl_show(struct seq_file *
 	return 0;
 }
 
-static ssize_t pgctrl_write(struct file *file, const char __user * buf,
-			    size_t count, loff_t * ppos)
+static ssize_t pgctrl_write(struct file *file, const char __user *buf,
+			    size_t count, loff_t *ppos)
 {
 	int err = 0;
 	char data[128];
@@ -545,11 +555,14 @@ static int pktgen_if_show(struct seq_fil
 			   "     daddr: %s  min_daddr: %s  max_daddr: %s\n", b1,
 			   b2, b3);
 
-	} else
+	} else {
+		seq_printf(seq,
+			   "     dst_min: %s  dst_max: %s\n",
+			   pkt_dev->dst_min, pkt_dev->dst_max);
 		seq_printf(seq,
-			   "     dst_min: %s  dst_max: %s\n     src_min: %s  src_max: %s\n",
-			   pkt_dev->dst_min, pkt_dev->dst_max, pkt_dev->src_min,
-			   pkt_dev->src_max);
+			   "        src_min: %s  src_max: %s\n",
+			   pkt_dev->src_min, pkt_dev->src_max);
+	}
 
 	seq_puts(seq, "     src_mac: ");
 
@@ -561,7 +574,8 @@ static int pktgen_if_show(struct seq_fil
 	seq_printf(seq, "%pM\n", pkt_dev->dst_mac);
 
 	seq_printf(seq,
-		   "     udp_src_min: %d  udp_src_max: %d  udp_dst_min: %d  udp_dst_max: %d\n",
+		   "     udp_src_min: %d  udp_src_max: %d"
+		   "  udp_dst_min: %d  udp_dst_max: %d\n",
 		   pkt_dev->udp_src_min, pkt_dev->udp_src_max,
 		   pkt_dev->udp_dst_min, pkt_dev->udp_dst_max);
 
@@ -577,23 +591,21 @@ static int pktgen_if_show(struct seq_fil
 				   i == pkt_dev->nr_labels-1 ? "\n" : ", ");
 	}
 
-	if (pkt_dev->vlan_id != 0xffff) {
+	if (pkt_dev->vlan_id != 0xffff)
 		seq_printf(seq, "     vlan_id: %u  vlan_p: %u  vlan_cfi: %u\n",
-			   pkt_dev->vlan_id, pkt_dev->vlan_p, pkt_dev->vlan_cfi);
-	}
+			   pkt_dev->vlan_id, pkt_dev->vlan_p,
+			   pkt_dev->vlan_cfi);
 
-	if (pkt_dev->svlan_id != 0xffff) {
+	if (pkt_dev->svlan_id != 0xffff)
 		seq_printf(seq, "     svlan_id: %u  vlan_p: %u  vlan_cfi: %u\n",
-			   pkt_dev->svlan_id, pkt_dev->svlan_p, pkt_dev->svlan_cfi);
-	}
+			   pkt_dev->svlan_id, pkt_dev->svlan_p,
+			   pkt_dev->svlan_cfi);
 
-	if (pkt_dev->tos) {
+	if (pkt_dev->tos)
 		seq_printf(seq, "     tos: 0x%02x\n", pkt_dev->tos);
-	}
 
-	if (pkt_dev->traffic_class) {
+	if (pkt_dev->traffic_class)
 		seq_printf(seq, "     traffic_class: 0x%02x\n", pkt_dev->traffic_class);
-	}
 
 	seq_printf(seq, "     Flags: ");
 
@@ -696,7 +708,8 @@ static int pktgen_if_show(struct seq_fil
 }
 
 
-static int hex32_arg(const char __user *user_buffer, unsigned long maxlen, __u32 *num)
+static int hex32_arg(const char __user *user_buffer, unsigned long maxlen,
+		     __u32 *num)
 {
 	int i = 0;
 	*num = 0;
@@ -846,9 +859,9 @@ static ssize_t pktgen_if_write(struct fi
 	/* Read variable name */
 
 	len = strn_len(&user_buffer[i], sizeof(name) - 1);
-	if (len < 0) {
+	if (len < 0)
 		return len;
-	}
+
 	memset(name, 0, sizeof(name));
 	if (copy_from_user(name, &user_buffer[i], len))
 		return -EFAULT;
@@ -872,9 +885,9 @@ static ssize_t pktgen_if_write(struct fi
 
 	if (!strcmp(name, "min_pkt_size")) {
 		len = num_arg(&user_buffer[i], 10, &value);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		i += len;
 		if (value < 14 + 20 + 8)
 			value = 14 + 20 + 8;
@@ -889,9 +902,9 @@ static ssize_t pktgen_if_write(struct fi
 
 	if (!strcmp(name, "max_pkt_size")) {
 		len = num_arg(&user_buffer[i], 10, &value);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		i += len;
 		if (value < 14 + 20 + 8)
 			value = 14 + 20 + 8;
@@ -908,9 +921,9 @@ static ssize_t pktgen_if_write(struct fi
 
 	if (!strcmp(name, "pkt_size")) {
 		len = num_arg(&user_buffer[i], 10, &value);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		i += len;
 		if (value < 14 + 20 + 8)
 			value = 14 + 20 + 8;
@@ -925,9 +938,9 @@ static ssize_t pktgen_if_write(struct fi
 
 	if (!strcmp(name, "debug")) {
 		len = num_arg(&user_buffer[i], 10, &value);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		i += len;
 		debug = value;
 		sprintf(pg_result, "OK: debug=%u", debug);
@@ -936,9 +949,9 @@ static ssize_t pktgen_if_write(struct fi
 
 	if (!strcmp(name, "frags")) {
 		len = num_arg(&user_buffer[i], 10, &value);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		i += len;
 		pkt_dev->nfrags = value;
 		sprintf(pg_result, "OK: frags=%u", pkt_dev->nfrags);
@@ -946,9 +959,9 @@ static ssize_t pktgen_if_write(struct fi
 	}
 	if (!strcmp(name, "delay")) {
 		len = num_arg(&user_buffer[i], 10, &value);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		i += len;
 		if (value == 0x7FFFFFFF)
 			pkt_dev->delay = ULLONG_MAX;
@@ -961,9 +974,9 @@ static ssize_t pktgen_if_write(struct fi
 	}
 	if (!strcmp(name, "udp_src_min")) {
 		len = num_arg(&user_buffer[i], 10, &value);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		i += len;
 		if (value != pkt_dev->udp_src_min) {
 			pkt_dev->udp_src_min = value;
@@ -974,9 +987,9 @@ static ssize_t pktgen_if_write(struct fi
 	}
 	if (!strcmp(name, "udp_dst_min")) {
 		len = num_arg(&user_buffer[i], 10, &value);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		i += len;
 		if (value != pkt_dev->udp_dst_min) {
 			pkt_dev->udp_dst_min = value;
@@ -987,9 +1000,9 @@ static ssize_t pktgen_if_write(struct fi
 	}
 	if (!strcmp(name, "udp_src_max")) {
 		len = num_arg(&user_buffer[i], 10, &value);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		i += len;
 		if (value != pkt_dev->udp_src_max) {
 			pkt_dev->udp_src_max = value;
@@ -1000,9 +1013,9 @@ static ssize_t pktgen_if_write(struct fi
 	}
 	if (!strcmp(name, "udp_dst_max")) {
 		len = num_arg(&user_buffer[i], 10, &value);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		i += len;
 		if (value != pkt_dev->udp_dst_max) {
 			pkt_dev->udp_dst_max = value;
@@ -1013,9 +1026,9 @@ static ssize_t pktgen_if_write(struct fi
 	}
 	if (!strcmp(name, "clone_skb")) {
 		len = num_arg(&user_buffer[i], 10, &value);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		i += len;
 		pkt_dev->clone_skb = value;
 
@@ -1024,9 +1037,9 @@ static ssize_t pktgen_if_write(struct fi
 	}
 	if (!strcmp(name, "count")) {
 		len = num_arg(&user_buffer[i], 10, &value);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		i += len;
 		pkt_dev->count = value;
 		sprintf(pg_result, "OK: count=%llu",
@@ -1035,9 +1048,9 @@ static ssize_t pktgen_if_write(struct fi
 	}
 	if (!strcmp(name, "src_mac_count")) {
 		len = num_arg(&user_buffer[i], 10, &value);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		i += len;
 		if (pkt_dev->src_mac_count != value) {
 			pkt_dev->src_mac_count = value;
@@ -1049,9 +1062,9 @@ static ssize_t pktgen_if_write(struct fi
 	}
 	if (!strcmp(name, "dst_mac_count")) {
 		len = num_arg(&user_buffer[i], 10, &value);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		i += len;
 		if (pkt_dev->dst_mac_count != value) {
 			pkt_dev->dst_mac_count = value;
@@ -1065,9 +1078,9 @@ static ssize_t pktgen_if_write(struct fi
 		char f[32];
 		memset(f, 0, 32);
 		len = strn_len(&user_buffer[i], sizeof(f) - 1);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		if (copy_from_user(f, &user_buffer[i], len))
 			return -EFAULT;
 		i += len;
@@ -1166,9 +1179,8 @@ static ssize_t pktgen_if_write(struct fi
 	}
 	if (!strcmp(name, "dst_min") || !strcmp(name, "dst")) {
 		len = strn_len(&user_buffer[i], sizeof(pkt_dev->dst_min) - 1);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
 
 		if (copy_from_user(buf, &user_buffer[i], len))
 			return -EFAULT;
@@ -1188,9 +1200,9 @@ static ssize_t pktgen_if_write(struct fi
 	}
 	if (!strcmp(name, "dst_max")) {
 		len = strn_len(&user_buffer[i], sizeof(pkt_dev->dst_max) - 1);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 
 		if (copy_from_user(buf, &user_buffer[i], len))
 			return -EFAULT;
@@ -1301,9 +1313,9 @@ static ssize_t pktgen_if_write(struct fi
 	}
 	if (!strcmp(name, "src_min")) {
 		len = strn_len(&user_buffer[i], sizeof(pkt_dev->src_min) - 1);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		if (copy_from_user(buf, &user_buffer[i], len))
 			return -EFAULT;
 		buf[len] = 0;
@@ -1322,9 +1334,9 @@ static ssize_t pktgen_if_write(struct fi
 	}
 	if (!strcmp(name, "src_max")) {
 		len = strn_len(&user_buffer[i], sizeof(pkt_dev->src_max) - 1);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		if (copy_from_user(buf, &user_buffer[i], len))
 			return -EFAULT;
 		buf[len] = 0;
@@ -1348,9 +1360,9 @@ static ssize_t pktgen_if_write(struct fi
 		memcpy(old_dmac, pkt_dev->dst_mac, ETH_ALEN);
 
 		len = strn_len(&user_buffer[i], sizeof(valstr) - 1);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		memset(valstr, 0, sizeof(valstr));
 		if (copy_from_user(valstr, &user_buffer[i], len))
 			return -EFAULT;
@@ -1390,9 +1402,9 @@ static ssize_t pktgen_if_write(struct fi
 		memcpy(old_smac, pkt_dev->src_mac, ETH_ALEN);
 
 		len = strn_len(&user_buffer[i], sizeof(valstr) - 1);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		memset(valstr, 0, sizeof(valstr));
 		if (copy_from_user(valstr, &user_buffer[i], len))
 			return -EFAULT;
@@ -1433,9 +1445,9 @@ static ssize_t pktgen_if_write(struct fi
 
 	if (!strcmp(name, "flows")) {
 		len = num_arg(&user_buffer[i], 10, &value);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		i += len;
 		if (value > MAX_CFLOWS)
 			value = MAX_CFLOWS;
@@ -1447,9 +1459,9 @@ static ssize_t pktgen_if_write(struct fi
 
 	if (!strcmp(name, "flowlen")) {
 		len = num_arg(&user_buffer[i], 10, &value);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		i += len;
 		pkt_dev->lflow = value;
 		sprintf(pg_result, "OK: flowlen=%u", pkt_dev->lflow);
@@ -1458,9 +1470,9 @@ static ssize_t pktgen_if_write(struct fi
 
 	if (!strcmp(name, "queue_map_min")) {
 		len = num_arg(&user_buffer[i], 5, &value);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		i += len;
 		pkt_dev->queue_map_min = value;
 		sprintf(pg_result, "OK: queue_map_min=%u", pkt_dev->queue_map_min);
@@ -1469,9 +1481,9 @@ static ssize_t pktgen_if_write(struct fi
 
 	if (!strcmp(name, "queue_map_max")) {
 		len = num_arg(&user_buffer[i], 5, &value);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		i += len;
 		pkt_dev->queue_map_max = value;
 		sprintf(pg_result, "OK: queue_map_max=%u", pkt_dev->queue_map_max);
@@ -1503,9 +1515,9 @@ static ssize_t pktgen_if_write(struct fi
 
 	if (!strcmp(name, "vlan_id")) {
 		len = num_arg(&user_buffer[i], 4, &value);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		i += len;
 		if (value <= 4095) {
 			pkt_dev->vlan_id = value;  /* turn on VLAN */
@@ -1530,9 +1542,9 @@ static ssize_t pktgen_if_write(struct fi
 
 	if (!strcmp(name, "vlan_p")) {
 		len = num_arg(&user_buffer[i], 1, &value);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		i += len;
 		if ((value <= 7) && (pkt_dev->vlan_id != 0xffff)) {
 			pkt_dev->vlan_p = value;
@@ -1545,9 +1557,9 @@ static ssize_t pktgen_if_write(struct fi
 
 	if (!strcmp(name, "vlan_cfi")) {
 		len = num_arg(&user_buffer[i], 1, &value);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		i += len;
 		if ((value <= 1) && (pkt_dev->vlan_id != 0xffff)) {
 			pkt_dev->vlan_cfi = value;
@@ -1560,9 +1572,9 @@ static ssize_t pktgen_if_write(struct fi
 
 	if (!strcmp(name, "svlan_id")) {
 		len = num_arg(&user_buffer[i], 4, &value);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		i += len;
 		if ((value <= 4095) && ((pkt_dev->vlan_id != 0xffff))) {
 			pkt_dev->svlan_id = value;  /* turn on SVLAN */
@@ -1587,9 +1599,9 @@ static ssize_t pktgen_if_write(struct fi
 
 	if (!strcmp(name, "svlan_p")) {
 		len = num_arg(&user_buffer[i], 1, &value);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		i += len;
 		if ((value <= 7) && (pkt_dev->svlan_id != 0xffff)) {
 			pkt_dev->svlan_p = value;
@@ -1602,9 +1614,9 @@ static ssize_t pktgen_if_write(struct fi
 
 	if (!strcmp(name, "svlan_cfi")) {
 		len = num_arg(&user_buffer[i], 1, &value);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		i += len;
 		if ((value <= 1) && (pkt_dev->svlan_id != 0xffff)) {
 			pkt_dev->svlan_cfi = value;
@@ -1618,9 +1630,9 @@ static ssize_t pktgen_if_write(struct fi
 	if (!strcmp(name, "tos")) {
 		__u32 tmp_value = 0;
 		len = hex32_arg(&user_buffer[i], 2, &tmp_value);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		i += len;
 		if (len == 2) {
 			pkt_dev->tos = tmp_value;
@@ -1634,9 +1646,9 @@ static ssize_t pktgen_if_write(struct fi
 	if (!strcmp(name, "traffic_class")) {
 		__u32 tmp_value = 0;
 		len = hex32_arg(&user_buffer[i], 2, &tmp_value);
-		if (len < 0) {
+		if (len < 0)
 			return len;
-		}
+
 		i += len;
 		if (len == 2) {
 			pkt_dev->traffic_class = tmp_value;
@@ -1906,13 +1918,14 @@ static int pktgen_device_event(struct no
 	return NOTIFY_DONE;
 }
 
-static struct net_device *pktgen_dev_get_by_name(struct pktgen_dev *pkt_dev, const char *ifname)
+static struct net_device *pktgen_dev_get_by_name(struct pktgen_dev *pkt_dev,
+						 const char *ifname)
 {
 	char b[IFNAMSIZ+5];
 	int i = 0;
 
-	for(i=0; ifname[i] != '@'; i++) {
-		if(i == IFNAMSIZ)
+	for (i = 0; ifname[i] != '@'; i++) {
+		if (i == IFNAMSIZ)
 			break;
 
 		b[i] = ifname[i];
@@ -1979,7 +1992,7 @@ static void pktgen_setup_inject(struct p
 		printk(KERN_WARNING "pktgen: WARNING: Requested "
 		       "queue_map_min (zero-based) (%d) exceeds valid range "
 		       "[0 - %d] for (%d) queues on %s, resetting\n",
-		       pkt_dev->queue_map_min, (ntxq ?: 1)- 1, ntxq,
+		       pkt_dev->queue_map_min, (ntxq ?: 1) - 1, ntxq,
 		       pkt_dev->odev->name);
 		pkt_dev->queue_map_min = ntxq - 1;
 	}
@@ -1987,7 +2000,7 @@ static void pktgen_setup_inject(struct p
 		printk(KERN_WARNING "pktgen: WARNING: Requested "
 		       "queue_map_max (zero-based) (%d) exceeds valid range "
 		       "[0 - %d] for (%d) queues on %s, resetting\n",
-		       pkt_dev->queue_map_max, (ntxq ?: 1)- 1, ntxq,
+		       pkt_dev->queue_map_max, (ntxq ?: 1) - 1, ntxq,
 		       pkt_dev->odev->name);
 		pkt_dev->queue_map_max = ntxq - 1;
 	}
@@ -2028,7 +2041,8 @@ static void pktgen_setup_inject(struct p
 			 */
 
 			rcu_read_lock();
-			if ((idev = __in6_dev_get(pkt_dev->odev)) != NULL) {
+			idev = __in6_dev_get(pkt_dev->odev);
+			if (idev) {
 				struct inet6_ifaddr *ifp;
 
 				read_lock_bh(&idev->lock);
@@ -2181,7 +2195,7 @@ static void get_ipsec_sa(struct pktgen_d
 		if (x) {
 			pkt_dev->flows[flow].x = x;
 			set_pkt_overhead(pkt_dev);
-			pkt_dev->pkt_overhead+=x->props.header_len;
+			pkt_dev->pkt_overhead += x->props.header_len;
 		}
 
 	}
@@ -2320,18 +2334,18 @@ static void mod_cur_headers(struct pktge
 
 	if (!(pkt_dev->flags & F_IPV6)) {
 
-		if ((imn = ntohl(pkt_dev->saddr_min)) < (imx =
-							 ntohl(pkt_dev->
-							       saddr_max))) {
+		imn = ntohl(pkt_dev->saddr_min);
+		imx = ntohl(pkt_dev->saddr_max);
+		if (imn < imx) {
 			__u32 t;
 			if (pkt_dev->flags & F_IPSRC_RND)
 				t = random32() % (imx - imn) + imn;
 			else {
 				t = ntohl(pkt_dev->cur_saddr);
 				t++;
-				if (t > imx) {
+				if (t > imx)
 					t = imn;
-				}
+
 			}
 			pkt_dev->cur_saddr = htonl(t);
 		}
@@ -2442,7 +2456,7 @@ static int pktgen_output_ipsec(struct sk
 	if (err)
 		goto error;
 
-	x->curlft.bytes +=skb->len;
+	x->curlft.bytes += skb->len;
 	x->curlft.packets++;
 error:
 	spin_unlock(&x->lock);
@@ -2474,11 +2488,11 @@ static int process_ipsec(struct pktgen_d
 			int ret;
 			__u8 *eth;
 			nhead = x->props.header_len - skb_headroom(skb);
-			if (nhead >0) {
+			if (nhead > 0) {
 				ret = pskb_expand_head(skb, nhead, 0, GFP_ATOMIC);
 				if (ret < 0) {
 					printk(KERN_ERR "Error expanding "
-					       "ipsec packet %d\n",ret);
+					       "ipsec packet %d\n", ret);
 					goto err;
 				}
 			}
@@ -2488,13 +2502,13 @@ static int process_ipsec(struct pktgen_d
 			ret = pktgen_output_ipsec(skb, pkt_dev);
 			if (ret) {
 				printk(KERN_ERR "Error creating ipsec "
-				       "packet %d\n",ret);
+				       "packet %d\n", ret);
 				goto err;
 			}
 			/* restore ll */
 			eth = (__u8 *) skb_push(skb, ETH_HLEN);
 			memcpy(eth, pkt_dev->hh, 12);
-			*(u16 *) & eth[12] = protocol;
+			*(u16 *) &eth[12] = protocol;
 		}
 	}
 	return 1;
@@ -2507,9 +2521,9 @@ err:
 static void mpls_push(__be32 *mpls, struct pktgen_dev *pkt_dev)
 {
 	unsigned i;
-	for (i = 0; i < pkt_dev->nr_labels; i++) {
+	for (i = 0; i < pkt_dev->nr_labels; i++)
 		*mpls++ = pkt_dev->labels[i] & ~MPLS_STACK_BOTTOM;
-	}
+
 	mpls--;
 	*mpls |= MPLS_STACK_BOTTOM;
 }
@@ -2676,8 +2690,9 @@ static struct sk_buff *fill_packet_ipv4(
 		}
 	}
 
-	/* Stamp the time, and sequence number, convert them to network byte order */
-
+	/* Stamp the time, and sequence number,
+	 * convert them to network byte order
+	 */
 	if (pgh) {
 		struct timeval timestamp;
 
@@ -2931,7 +2946,7 @@ static struct sk_buff *fill_packet_ipv6(
 	udph = udp_hdr(skb);
 
 	memcpy(eth, pkt_dev->hh, 12);
-	*(__be16 *) & eth[12] = protocol;
+	*(__be16 *) &eth[12] = protocol;
 
 	/* Eth + IPh + UDPh + mpls */
 	datalen = pkt_dev->cur_pkt_size - 14 -
@@ -3025,8 +3040,10 @@ static struct sk_buff *fill_packet_ipv6(
 		}
 	}
 
-	/* Stamp the time, and sequence number, convert them to network byte order */
-	/* should we update cloned packets too ? */
+	/* Stamp the time, and sequence number,
+	 * convert them to network byte order
+	 * should we update cloned packets too ?
+	 */
 	if (pgh) {
 		struct timeval timestamp;
 
@@ -3174,7 +3191,8 @@ static void pktgen_run_all_threads(void)
 
 	mutex_unlock(&pktgen_thread_lock);
 
-	schedule_timeout_interruptible(msecs_to_jiffies(125));	/* Propagate thread->control  */
+	/* Propagate thread->control  */
+	schedule_timeout_interruptible(msecs_to_jiffies(125));
 
 	pktgen_wait_all_threads_run();
 }
@@ -3192,7 +3210,8 @@ static void pktgen_reset_all_threads(voi
 
 	mutex_unlock(&pktgen_thread_lock);
 
-	schedule_timeout_interruptible(msecs_to_jiffies(125));	/* Propagate thread->control  */
+	/* Propagate thread->control  */
+	schedule_timeout_interruptible(msecs_to_jiffies(125));
 
 	pktgen_wait_all_threads_run();
 }
@@ -3485,7 +3504,8 @@ static int pktgen_thread_worker(void *ar
 	init_waitqueue_head(&t->queue);
 	complete(&t->start_done);
 
-	pr_debug("pktgen: starting pktgen/%d:  pid=%d\n", cpu, task_pid_nr(current));
+	pr_debug("pktgen: starting pktgen/%d:  pid=%d\n",
+		 cpu, task_pid_nr(current));
 
 	set_current_state(TASK_INTERRUPTIBLE);
 
#20 lore raw

PATCH 14/14 pktgen: increase version

In reply to Stephen Hemminger

Increase module version, and cleanup module info.

net/core/pktgen.c +10 -6
@@ -169,9 +169,7 @@
 #include <asm/dma.h>
 #include <asm/div64.h>		/* do_div */
 
-#define VERSION \
-	"pktgen v2.70: Packet Generator for packet performance testing.\n"
-
+#define VERSION 	"2.72"
 #define IP_NAME_SZ 32
 #define MAX_MPLS_LABELS 16 /* This is the max label stack depth */
 #define MPLS_STACK_BOTTOM htonl(0x00000100)
@@ -422,7 +420,8 @@ static inline int ktime_lt(const ktime_t
 	return cmp1.tv64 < cmp2.tv64;
 }
 
-static const char version[] __initconst = VERSION;
+static const char version[] =
+	"pktgen " VERSION ": Packet Generator for packet performance testing.\n";
 
 static int pktgen_remove_device(struct pktgen_thread *t, struct pktgen_dev *i);
 static int pktgen_add_device(struct pktgen_thread *t, const char *ifname);
@@ -459,7 +458,7 @@ static struct notifier_block pktgen_noti
 
 static int pgctrl_show(struct seq_file *seq, void *v)
 {
-	seq_puts(seq, VERSION);
+	seq_puts(seq, version);
 	return 0;
 }
 
@@ -3852,10 +3851,15 @@ static void __exit pg_cleanup(void)
 module_init(pg_init);
 module_exit(pg_cleanup);
 
-MODULE_AUTHOR("Robert Olsson <[email protected]");
+MODULE_AUTHOR("Robert Olsson <[email protected]>");
 MODULE_DESCRIPTION("Packet Generator tool");
 MODULE_LICENSE("GPL");
+MODULE_VERSION(VERSION);
 module_param(pg_count_d, int, 0);
+MODULE_PARM_DESC(pg_count_d, "Default number of packets to inject");
 module_param(pg_delay_d, int, 0);
+MODULE_PARM_DESC(pg_delay_d, "Default delay between packets (nanoseconds)");
 module_param(pg_clone_skb_d, int, 0);
+MODULE_PARM_DESC(pg_clone_skb_d, "Default number of copies of the same packet");
 module_param(debug, int, 0);
+MODULE_PARM_DESC(debug, "Enable debugging of pktgen module");
maintainer
#21 lore raw
In reply to Stephen Hemminger

From: Stephen Hemminger <[email protected]>
Date: Thu, 27 Aug 2009 16:55:06 -0700

Quoted 2 lines

The biggest change is switching to monotonic clock (ktime) and high resolution timers for the interpacket delay.

Ok, even though there were some issues with the TX return values wrt. VLANS, I've applied all of this.

The cleanups were good and hrtimers and ktime_t are definitely the direction this code should go in.

It would be good if this new stuff got a lot of testing and debugging though.

maintainer
#22 lore raw

From: David Miller <[email protected]>
Date: Fri, 28 Aug 2009 23:33:20 -0700 (PDT)

Quoted 6 lines

From: Stephen Hemminger <[email protected]>
Date: Thu, 27 Aug 2009 16:55:06 -0700

Quoted 2 lines

The biggest change is switching to monotonic clock (ktime) and high resolution timers for the interpacket delay.

Quoted 8 lines

Ok, even though there were some issues with the TX return values wrt. VLANS, I've applied all of this.

The cleanups were good and hrtimers and ktime_t are definitely the direction this code should go in.

It would be good if this new stuff got a lot of testing and debugging though.

BTW, to make this build in all cases I had to add a symbol export for hrtimer_init_on_stack().

#23 lore raw

cover pktgen update for net-next (2.6.32)

In reply to Stephen Hemminger
Quoted 2 lines · Stephen Hemminger

The biggest change is switching to monotonic clock (ktime) and high resolution timers for the interpacket delay.

Opteron Barcelona 2.3 GHz with net-next 2.6.31-rc5bifrost-x86_64.

Sending different pkt-sizes at maxrate using one CPU-core (of eight) with ixgbe driver and Intel's 82598 chip.

w. patch

64    4761904    3846153
128   4545454    3703703
256   4545454    3703703
512   2380952    2380952
1024  1204819    1204819

1500 826446 826446

We increase max sending rate from 3.8 Mpps to 4.8 Mpps for 64 byte pkts

Thanks a lot Stephen.

Cheers

                                        --ro
#24 lore raw
Quoted 10 lines · On Tue, 8 Sep 2009, [email protected] wrote:
Quoted 2 lines · Stephen Hemminger

The biggest change is switching to monotonic clock (ktime) and high resolution timers for the interpacket delay.

Opteron Barcelona 2.3 GHz with net-next 2.6.31-rc5bifrost-x86_64.

Sending different pkt-sizes at maxrate using one CPU-core (of eight) with ixgbe driver and Intel's 82598 chip.

w. patch

Quoted 8 lines
64    4761904    3846153
128   4545454    3703703
256   4545454    3703703
512   2380952    2380952
1024  1204819    1204819

1500 826446 826446

We increase max sending rate from 3.8 Mpps to 4.8 Mpps for 64 byte pkts

I can also measure a performance improvement.

Generator machine, AMD Phenom 9950 quad core, using a 10GbE Intel 82599 NIC (6 port 10GbE from Hotlava Systems Inc.).

With patches:
-------------
tx_pkt_sz: 64 TX-pps: 9426724
tx_pkt_sz: 128 TX-pps: 8220396
tx_pkt_sz: 256 TX-pps: 4463415
tx_pkt_sz: 384 TX-pps: 3063025
tx_pkt_sz: 512 TX-pps: 2331636
tx_pkt_sz: 640 TX-pps: 1879640
tx_pkt_sz: 768 TX-pps: 1577957
tx_pkt_sz: 896 TX-pps: 1359122
tx_pkt_sz: 1024 TX-pps: 1193118
tx_pkt_sz: 1152 TX-pps: 1062726
tx_pkt_sz: 1280 TX-pps: 958408
tx_pkt_sz: 1408 TX-pps: 873167
tx_pkt_sz: 1514 TX-pps: 812591

Without patches:
----------------
tx_pkt_sz: 64 TX-pps: 8372361
tx_pkt_sz: 128 TX-pps: 7915717
tx_pkt_sz: 256 TX-pps: 4462400
tx_pkt_sz: 384 TX-pps: 3059712
tx_pkt_sz: 512 TX-pps: 2332164
tx_pkt_sz: 640 TX-pps: 1882343
tx_pkt_sz: 768 TX-pps: 1577543
tx_pkt_sz: 896 TX-pps: 1358061
tx_pkt_sz: 1024 TX-pps: 1192788
tx_pkt_sz: 1152 TX-pps: 1062959
tx_pkt_sz: 1280 TX-pps: 958622
tx_pkt_sz: 1408 TX-pps: 872934
tx_pkt_sz: 1514 TX-pps: 812771

It really makes a difference for small packet sizes.

64 bytes pkts: 8.4 Mpps to 9.4 Mpps.
128 bytes pkts: 7.9 Mpps to 8.2 Mpps

Notice that the numbers for pkt sizes above 128 bytes are really close to wirespeed 10GbE rates, when taking ethernet framing into account.

Quoted 1 line

Thanks a lot Stephen.

Yes, thanks a lot Stephen! :-)

Cheers,
Jesper Brouer

--
-------------------------------------------------------------------
MSc. Master of Computer Science
Dept. of Computer Science, University of Copenhagen Author of http://www.adsl-optimizer.dk
-------------------------------------------------------------------

#25 lore raw
Quoted 7 lines · On Tue, 8 Sep 2009, [email protected] wrote:
Quoted 6 lines

Sending different pkt-sizes at maxrate using one CPU-core (of eight) with ixgbe driver and Intel's 82598 chip.

w. patch
64 4761904 3846153

Quoted 8 lines

I can also measure a performance improvement.

Generator machine, AMD Phenom 9950 quad core, using a 10GbE Intel 82599 NIC (6 port 10GbE from Hotlava Systems Inc.).

With patches:
-------------
tx_pkt_sz: 64 TX-pps: 9426724

Impressive. 82599 seems fast. Didn't you report even higher number with other CPU's?

Cheers

					--ro
#26 lore raw
Quoted 6 lines · On Tue, 8 Sep 2009, [email protected] wrote:
Quoted 3 lines

With patches:
-------------
tx_pkt_sz: 64 TX-pps: 9426724

Impressive. 82599 seems fast. Didn't you report even higher number with other CPU's?

Yes, the Core i7 is even faster... but currently I'm not using it as a generator, its the router in my current setup.

Once I have finished my slides and last tests for LinuxCon. I'll run some pktgen tests with the Core i7 and post the results.

Cheers,
Jesper Brouer

--
-------------------------------------------------------------------
MSc. Master of Computer Science
Dept. of Computer Science, University of Copenhagen Author of http://www.adsl-optimizer.dk
-------------------------------------------------------------------

#27 lore raw
Quoted 6 lines · Jesper Dangaard Brouer
Quoted 4 lines · On Tue, 8 Sep 2009, [email protected] wrote:
Quoted 3 lines

With patches:
-------------
tx_pkt_sz: 64 TX-pps: 9426724

Quoted 6 lines
Quoted 2 lines

Impressive. 82599 seems fast. Didn't you report even higher number with other CPU's?

Yes, the Core i7 is even faster... but currently I'm not using it as a generator, its the router in my current setup.

Once I have finished my slides and last tests for LinuxCon. I'll run some pktgen tests with the Core i7 and post the results.

Here are some quick test results:

One thing I noticed during the tests, is that using the machine during the tests gets very sluggy with the pktgen patches applied.

Another thing/bug that sometimes happens is that pktgen stops working and the ixgbe driver "says":

[  231.195624] pktgen 2.72: Packet Generator for packet performance testing.
[  233.065639] ixgbe 0000:07:00.1: master disable timed out
[  234.677107] ixgbe 0000:07:00.1: master disable timed out
[  236.284576] ixgbe 0000:07:00.1: master disable timed out
[  237.892045] ixgbe 0000:07:00.1: master disable timed out

The tests, I did complete, gave the following results:

Core i7-920 (with DDR3 at 1600Mhz)
----------------------------------
Without patches (2.6.31-rc2):

 tx_pkt_sz: 64   TX-Mbps: 5704.29    TX-pps: 11141193
 tx_pkt_sz: 128  TX-Mbps: 8415.27    TX-pps: 8218035
 tx_pkt_sz: 256  TX-Mbps: 9146.23    TX-pps: 4465932

With the pktgen patches (2.6.31-rc5):

 tx_pkt_sz: 64   TX-Mbps: 5763.64        TX-pps: 11257313
 tx_pkt_sz: 128  TX-Mbps: 8414.91        TX-pps: 8217709
 tx_pkt_sz: 256  TX-Mbps: 9141.70        TX-pps: 4463720

Cheers,
Jesper Brouer

--
-------------------------------------------------------------------
MSc. Master of Computer Science
Dept. of Computer Science, University of Copenhagen Author of http://www.adsl-optimizer.dk
-------------------------------------------------------------------