[PATCH net-next] pktgen: Fill the payload optionally with a pattern

[PATCH net-next] pktgen: Fill the payload optionally with a pattern

[Zoltan Kiss]

Introduces a new flag called PATTERN, which puts a non-periodic, predicatble
pattern into the payload. This was useful to reproduce an otherwise intermittent
bug in xen-netback [1], where checksum checking doesn't help.
The pattern is a repetition of " %lu", a series of increasing numbers divided by
space. The value of the number is the size of the preceding payload area. E.g.
" 1 3 5"..." 1000 1005 1010"
If the pattern is used, every frag will have its own page, unlike before, so it
needs more memory.

[1] 5837574: xen-netback: Fix grant ref resolution in RX path

Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Thomas Graf <tgraf@suug.ch>
Cc: Joe Perches <joe@perches.com>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: xen-devel@lists.xenproject.org

---
 net/core/pktgen.c |  131 +++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 118 insertions(+), 13 deletions(-)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 0304f98..69eb482 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -201,6 +201,7 @@
 #define F_QUEUE_MAP_CPU (1<<14)	/* queue map mirrors smp_processor_id() */
 #define F_NODE          (1<<15)	/* Node memory alloc*/
 #define F_UDPCSUM       (1<<16)	/* Include UDP checksum */
+#define F_PATTERN       (1<<17)	/* Fill the payload with a pattern */
 
 /* Thread control flag bits */
 #define T_STOP        (1<<0)	/* Stop run */
@@ -255,7 +256,7 @@ struct pktgen_dev {
 	int max_pkt_size;
 	int pkt_overhead;	/* overhead for MPLS, VLANs, IPSEC etc */
 	int nfrags;
-	struct page *page;
+	struct page *pages[MAX_SKB_FRAGS];
 	u64 delay;		/* nano-seconds */
 
 	__u64 count;		/* Default No packets to send */
@@ -1127,11 +1128,13 @@ static ssize_t pktgen_if_write(struct file *file,
 		i += len;
 
 		if (node_possible(value)) {
+			int j;
 			pkt_dev->node = value;
 			sprintf(pg_result, "OK: node=%d", pkt_dev->node);
-			if (pkt_dev->page) {
-				put_page(pkt_dev->page);
-				pkt_dev->page = NULL;
+			for (j = 0; j < MAX_SKB_FRAGS; ++j)
+				if (pkt_dev->pages[j]) {
+					put_page(pkt_dev->pages[j]);
+					pkt_dev->pages[j] = NULL;
 			}
 		}
 		else
@@ -1242,6 +1245,12 @@ static ssize_t pktgen_if_write(struct file *file,
 		else if (strcmp(f, "!UDPCSUM") == 0)
 			pkt_dev->flags &= ~F_UDPCSUM;
 
+		else if (strcmp(f, "PATTERN") == 0)
+			pkt_dev->flags |= F_PATTERN;
+
+		else if (strcmp(f, "!PATTERN") == 0)
+			pkt_dev->flags &= ~F_PATTERN;
+
 		else {
 			sprintf(pg_result,
 				"Flag -:%s:- unknown\nAvailable flags, (prepend ! to un-set flag):\n%s",
@@ -2623,17 +2632,90 @@ static inline __be16 build_tci(unsigned int id, unsigned int cfi,
 	return htons(id | (cfi << 12) | (prio << 13));
 }
 
+/* Max number of digits. The sizeof equals to log base 2^8 (UINT_MAX), multiply
+ * with 3 is a cheap, rounded up conversion to log10
+ */
+#define UINT_MAX_DIGITS (3*sizeof(unsigned long)+1)
+
+/* Fill the buffer up with a pattern
+ * buf - pointer to buffer
+ * bufsize - size of the buffer
+ * start - starting value for the pattern
+ * incomplete - pointer to the offset inside the pattern, or 0 if none
+ *
+ * The pattern is a repetition of " %lu", a series of increasing numbers divided
+ * by space. The value of the number is "start" plus the size of the preceding
+ * space. E.g. if start is 1000, it starts like " 1000 1005 1010".
+ * If the last number doesn't fit, it gets truncated, and the number of leading
+ * characters is saved into incomplete. It can be passed then to the next call
+ * with the next buffer, and the pattern looks contiguous over scattered
+ * buffers.
+ * It returns "start" plus the offset of the byte after the last full
+ * pattern write.
+ */
+static unsigned long pattern_to_packet(char *buf,
+				       int bufsize,
+				       unsigned long start,
+				       unsigned int *incomplete)
+{
+	int len;
+	/* Only used when the pattern doesn't align to the buffer */
+	char temp[UINT_MAX_DIGITS];
+
+	if (*incomplete) {
+		int copylen;
+
+		len = snprintf(temp, UINT_MAX_DIGITS, " %lu", start);
+		copylen = len - *incomplete;
+		if (copylen > bufsize) {
+			/* The continuation of this number couldn't fit here */
+			copylen = bufsize;
+			*incomplete += bufsize;
+		} else {
+			*incomplete = 0;
+			start += len;
+		}
+		memcpy(buf, temp + *incomplete, copylen);
+		bufsize -= copylen;
+		buf += copylen;
+	}
+
+	while (bufsize > 0) {
+		len = snprintf(buf, bufsize, " %lu", start);
+		/* The last number doesn't fit, remember where it was truncated.
+		 */
+		if (len >= bufsize) {
+			/* snprintf always add a trailing zero */
+			len = snprintf(temp, UINT_MAX_DIGITS, " %lu", start);
+			memcpy(buf, temp, bufsize);
+			*incomplete = len - bufsize;
+			return start;
+		}
+		bufsize -= len;
+		start += len;
+		buf += len;
+	}
+	return start;
+}
+
 static void pktgen_finalize_skb(struct pktgen_dev *pkt_dev, struct sk_buff *skb,
 				int datalen)
 {
 	struct timeval timestamp;
 	struct pktgen_hdr *pgh;
+	unsigned long offset = 0;
+	unsigned int incomplete = 0;
 
 	pgh = (struct pktgen_hdr *)skb_put(skb, sizeof(*pgh));
 	datalen -= sizeof(*pgh);
 
 	if (pkt_dev->nfrags <= 0) {
-		memset(skb_put(skb, datalen), 0, datalen);
+		if (pkt_dev->flags & F_PATTERN)
+			offset = pattern_to_packet(skb_put(skb, datalen),
+						   datalen, offset,
+						   &incomplete);
+		else
+			memset(skb_put(skb, datalen), 0, datalen);
 	} else {
 		int frags = pkt_dev->nfrags;
 		int i, len;
@@ -2644,7 +2726,12 @@ static void pktgen_finalize_skb(struct pktgen_dev *pkt_dev, struct sk_buff *skb,
 			frags = MAX_SKB_FRAGS;
 		len = datalen - frags * PAGE_SIZE;
 		if (len > 0) {
-			memset(skb_put(skb, len), 0, len);
+			if (pkt_dev->flags & F_PATTERN)
+				offset = pattern_to_packet(skb_put(skb, len),
+							   len, offset,
+							   &incomplete);
+			else
+				memset(skb_put(skb, len), 0, len);
 			datalen = frags * PAGE_SIZE;
 		}
 
@@ -2652,17 +2739,28 @@ static void pktgen_finalize_skb(struct pktgen_dev *pkt_dev, struct sk_buff *skb,
 		frag_len = (datalen/frags) < PAGE_SIZE ?
 			   (datalen/frags) : PAGE_SIZE;
 		while (datalen > 0) {
-			if (unlikely(!pkt_dev->page)) {
+			int fragpage;
+			gfp_t flags = GFP_KERNEL;
+
+			if (pkt_dev->flags & F_PATTERN) {
+				fragpage = i;
+			} else {
+				fragpage = 0;
+				flags |= __GFP_ZERO;
+			}
+
+			if (unlikely(!pkt_dev->pages[fragpage])) {
 				int node = numa_node_id();
 
 				if (pkt_dev->node >= 0 && (pkt_dev->flags & F_NODE))
 					node = pkt_dev->node;
-				pkt_dev->page = alloc_pages_node(node, GFP_KERNEL | __GFP_ZERO, 0);
-				if (!pkt_dev->page)
+				pkt_dev->pages[fragpage] = alloc_pages_node(node, flags, 0);
+				if (!pkt_dev->pages[fragpage])
 					break;
 			}
-			get_page(pkt_dev->page);
-			skb_frag_set_page(skb, i, pkt_dev->page);
+			get_page(pkt_dev->pages[fragpage]);
+			skb_frag_set_page(skb, i, pkt_dev->pages[fragpage]);
+
 			skb_shinfo(skb)->frags[i].page_offset = 0;
 			/*last fragment, fill rest of data*/
 			if (i == (frags - 1))
@@ -2671,6 +2769,10 @@ static void pktgen_finalize_skb(struct pktgen_dev *pkt_dev, struct sk_buff *skb,
 			else
 				skb_frag_size_set(&skb_shinfo(skb)->frags[i], frag_len);
 			datalen -= skb_frag_size(&skb_shinfo(skb)->frags[i]);
+			if (pkt_dev->flags & F_PATTERN)
+				offset = pattern_to_packet(skb_frag_address(&skb_shinfo(skb)->frags[i]),
+							   skb_frag_size(&skb_shinfo(skb)->frags[i]),
+							   offset, &incomplete);
 			skb->len += skb_frag_size(&skb_shinfo(skb)->frags[i]);
 			skb->data_len += skb_frag_size(&skb_shinfo(skb)->frags[i]);
 			i++;
@@ -3685,6 +3787,8 @@ static void _rem_dev_from_if_list(struct pktgen_thread *t,
 static int pktgen_remove_device(struct pktgen_thread *t,
 				struct pktgen_dev *pkt_dev)
 {
+	int i;
+
 	pr_debug("remove_device pkt_dev=%p\n", pkt_dev);
 
 	if (pkt_dev->running) {
@@ -3710,8 +3814,9 @@ static int pktgen_remove_device(struct pktgen_thread *t,
 	free_SAs(pkt_dev);
 #endif
 	vfree(pkt_dev->flows);
-	if (pkt_dev->page)
-		put_page(pkt_dev->page);
+	for (i = 0; i < MAX_SKB_FRAGS; ++i)
+		if (pkt_dev->pages[i])
+			put_page(pkt_dev->pages[i]);
 	kfree(pkt_dev);
 	return 0;
 }

[PATCH net-next] pktgen: Fill the payload optionally with a pattern

[Zoltan Kiss]

Introduces a new flag called PATTERN, which puts a non-periodic, predicatble
pattern into the payload. This was useful to reproduce an otherwise intermittent
bug in xen-netback [1], where checksum checking doesn't help.
The pattern is a repetition of " %lu", a series of increasing numbers divided by
space. The value of the number is the size of the preceding payload area. E.g.
" 1 3 5"..." 1000 1005 1010"
If the pattern is used, every frag will have its own page, unlike before, so it
needs more memory.

[1] 5837574: xen-netback: Fix grant ref resolution in RX path

Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Thomas Graf <tgraf@suug.ch>
Cc: Joe Perches <joe@perches.com>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: xen-devel@lists.xenproject.org
---
v2: some bugfixes for pattern_to_packet, as my upcoming patch revealed some

 net/core/pktgen.c |  139 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 126 insertions(+), 13 deletions(-)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 0304f98..253b83d 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -201,6 +201,7 @@
 #define F_QUEUE_MAP_CPU (1<<14)	/* queue map mirrors smp_processor_id() */
 #define F_NODE          (1<<15)	/* Node memory alloc*/
 #define F_UDPCSUM       (1<<16)	/* Include UDP checksum */
+#define F_PATTERN       (1<<17)	/* Fill the payload with a pattern */
 
 /* Thread control flag bits */
 #define T_STOP        (1<<0)	/* Stop run */
@@ -255,7 +256,7 @@ struct pktgen_dev {
 	int max_pkt_size;
 	int pkt_overhead;	/* overhead for MPLS, VLANs, IPSEC etc */
 	int nfrags;
-	struct page *page;
+	struct page *pages[MAX_SKB_FRAGS];
 	u64 delay;		/* nano-seconds */
 
 	__u64 count;		/* Default No packets to send */
@@ -1127,11 +1128,13 @@ static ssize_t pktgen_if_write(struct file *file,
 		i += len;
 
 		if (node_possible(value)) {
+			int j;
 			pkt_dev->node = value;
 			sprintf(pg_result, "OK: node=%d", pkt_dev->node);
-			if (pkt_dev->page) {
-				put_page(pkt_dev->page);
-				pkt_dev->page = NULL;
+			for (j = 0; j < MAX_SKB_FRAGS; ++j)
+				if (pkt_dev->pages[j]) {
+					put_page(pkt_dev->pages[j]);
+					pkt_dev->pages[j] = NULL;
 			}
 		}
 		else
@@ -1242,6 +1245,12 @@ static ssize_t pktgen_if_write(struct file *file,
 		else if (strcmp(f, "!UDPCSUM") == 0)
 			pkt_dev->flags &= ~F_UDPCSUM;
 
+		else if (strcmp(f, "PATTERN") == 0)
+			pkt_dev->flags |= F_PATTERN;
+
+		else if (strcmp(f, "!PATTERN") == 0)
+			pkt_dev->flags &= ~F_PATTERN;
+
 		else {
 			sprintf(pg_result,
 				"Flag -:%s:- unknown\nAvailable flags, (prepend ! to un-set flag):\n%s",
@@ -2623,17 +2632,98 @@ static inline __be16 build_tci(unsigned int id, unsigned int cfi,
 	return htons(id | (cfi << 12) | (prio << 13));
 }
 
+/* Max number of digits. The sizeof equals to log base 2^8 (UINT_MAX), multiply
+ * with 3 is a cheap, rounded up conversion to log10
+ */
+#define UINT_MAX_DIGITS (3*sizeof(unsigned long)+1)
+
+/* Fill the buffer up with a pattern
+ * buf - pointer to buffer
+ * bufsize - size of the buffer
+ * start - starting value for the pattern
+ * incomplete - pointer to the offset inside the pattern, or 0 if none
+ *
+ * The pattern is a repetition of " %lu", a series of increasing numbers divided
+ * by space. The value of the number is "start" plus the size of the preceding
+ * space. E.g. if start is 1000, it starts like " 1000 1005 1010".
+ * If the last number doesn't fit, it gets truncated, and the number of leading
+ * characters is saved into incomplete. It can be passed then to the next call
+ * with the next buffer, and the pattern looks contiguous over scattered
+ * buffers.
+ * It returns "start" plus the offset of the byte after the last full
+ * pattern write.
+ */
+static unsigned long pattern_to_packet(char *buf,
+				       int bufsize,
+				       unsigned long start,
+				       unsigned int *incomplete)
+{
+	int len;
+	/* Only used when the pattern doesn't align to the buffer */
+	char temp[UINT_MAX_DIGITS];
+
+	if (*incomplete) {
+		int copylen, offset = *incomplete;
+
+		len = snprintf(temp, sizeof(temp), " %lu", start);
+		copylen = len - *incomplete;
+		if (copylen > bufsize) {
+			/* The continuation of this number couldn't fit here */
+			copylen = bufsize;
+			*incomplete += bufsize;
+		} else {
+			*incomplete = 0;
+			start += len;
+		}
+		memcpy(buf, temp + offset, copylen);
+		bufsize -= copylen;
+		buf += copylen;
+	}
+
+	while (bufsize > 0) {
+		len = snprintf(buf, bufsize, " %lu", start);
+		/* The last number doesn't fit, remember where it was truncated.
+		 */
+		if (len >= bufsize) {
+			/* snprintf always add a trailing zero, but actually we
+			 * need the last digit there
+			 */
+			len = snprintf(temp, sizeof(temp), " %lu", start);
+			memcpy(buf, temp, bufsize);
+			/* If the last number just fit without the trailing
+			 * zero, the next buffer can continue from an increased
+			 * offset.
+			 */
+			if (len == bufsize)
+				start += len;
+			*incomplete = bufsize;
+			return start;
+		}
+		bufsize -= len;
+		start += len;
+		buf += len;
+	}
+	return start;
+}
+
 static void pktgen_finalize_skb(struct pktgen_dev *pkt_dev, struct sk_buff *skb,
 				int datalen)
 {
 	struct timeval timestamp;
 	struct pktgen_hdr *pgh;
+	unsigned long offset = 0;
+	unsigned int incomplete = 0;
 
 	pgh = (struct pktgen_hdr *)skb_put(skb, sizeof(*pgh));
 	datalen -= sizeof(*pgh);
 
 	if (pkt_dev->nfrags <= 0) {
-		memset(skb_put(skb, datalen), 0, datalen);
+		if (pkt_dev->flags & F_PATTERN)
+			offset = pattern_to_packet(skb_put(skb, datalen),
+						   datalen, offset,
+						   &incomplete);
+		else
+			memset(skb_put(skb, datalen), 0, datalen);
 	} else {
 		int frags = pkt_dev->nfrags;
 		int i, len;
@@ -2644,7 +2734,12 @@ static void pktgen_finalize_skb(struct pktgen_dev *pkt_dev, struct sk_buff *skb,
 			frags = MAX_SKB_FRAGS;
 		len = datalen - frags * PAGE_SIZE;
 		if (len > 0) {
-			memset(skb_put(skb, len), 0, len);
+			if (pkt_dev->flags & F_PATTERN)
+				offset = pattern_to_packet(skb_put(skb, len),
+							   len, offset,
+							   &incomplete);
+			else
+				memset(skb_put(skb, len), 0, len);
 			datalen = frags * PAGE_SIZE;
 		}
 
@@ -2652,17 +2747,28 @@ static void pktgen_finalize_skb(struct pktgen_dev *pkt_dev, struct sk_buff *skb,
 		frag_len = (datalen/frags) < PAGE_SIZE ?
 			   (datalen/frags) : PAGE_SIZE;
 		while (datalen > 0) {
-			if (unlikely(!pkt_dev->page)) {
+			int fragpage;
+			gfp_t flags = GFP_KERNEL;
+
+			if (pkt_dev->flags & F_PATTERN) {
+				fragpage = i;
+			} else {
+				fragpage = 0;
+				flags |= __GFP_ZERO;
+			}
+
+			if (unlikely(!pkt_dev->pages[fragpage])) {
 				int node = numa_node_id();
 
 				if (pkt_dev->node >= 0 && (pkt_dev->flags & F_NODE))
 					node = pkt_dev->node;
-				pkt_dev->page = alloc_pages_node(node, GFP_KERNEL | __GFP_ZERO, 0);
-				if (!pkt_dev->page)
+				pkt_dev->pages[fragpage] = alloc_pages_node(node, flags, 0);
+				if (!pkt_dev->pages[fragpage])
 					break;
 			}
-			get_page(pkt_dev->page);
-			skb_frag_set_page(skb, i, pkt_dev->page);
+			get_page(pkt_dev->pages[fragpage]);
+			skb_frag_set_page(skb, i, pkt_dev->pages[fragpage]);
+
 			skb_shinfo(skb)->frags[i].page_offset = 0;
 			/*last fragment, fill rest of data*/
 			if (i == (frags - 1))
@@ -2671,6 +2777,10 @@ static void pktgen_finalize_skb(struct pktgen_dev *pkt_dev, struct sk_buff *skb,
 			else
 				skb_frag_size_set(&skb_shinfo(skb)->frags[i], frag_len);
 			datalen -= skb_frag_size(&skb_shinfo(skb)->frags[i]);
+			if (pkt_dev->flags & F_PATTERN)
+				offset = pattern_to_packet(skb_frag_address(&skb_shinfo(skb)->frags[i]),
+							   skb_frag_size(&skb_shinfo(skb)->frags[i]),
+							   offset, &incomplete);
 			skb->len += skb_frag_size(&skb_shinfo(skb)->frags[i]);
 			skb->data_len += skb_frag_size(&skb_shinfo(skb)->frags[i]);
 			i++;
@@ -3685,6 +3795,8 @@ static void _rem_dev_from_if_list(struct pktgen_thread *t,
 static int pktgen_remove_device(struct pktgen_thread *t,
 				struct pktgen_dev *pkt_dev)
 {
+	int i;
+
 	pr_debug("remove_device pkt_dev=%p\n", pkt_dev);
 
 	if (pkt_dev->running) {
@@ -3710,8 +3822,9 @@ static int pktgen_remove_device(struct pktgen_thread *t,
 	free_SAs(pkt_dev);
 #endif
 	vfree(pkt_dev->flows);
-	if (pkt_dev->page)
-		put_page(pkt_dev->page);
+	for (i = 0; i < MAX_SKB_FRAGS; ++i)
+		if (pkt_dev->pages[i])
+			put_page(pkt_dev->pages[i]);
 	kfree(pkt_dev);
 	return 0;
 }

Re: [PATCH net-next v2] pktgen: Fill the payload optionally with a pattern

[Zoltan Kiss]

Sorry, I forgot the v2 from the subject

Zoltan

On 24/06/14 21:40, Zoltan Kiss wrote:
> Introduces a new flag called PATTERN, which puts a non-periodic, predicatble
> pattern into the payload. This was useful to reproduce an otherwise intermittent
> bug in xen-netback [1], where checksum checking doesn't help.
> The pattern is a repetition of " %lu", a series of increasing numbers divided by
> space. The value of the number is the size of the preceding payload area. E.g.
> " 1 3 5"..." 1000 1005 1010"
> If the pattern is used, every frag will have its own page, unlike before, so it
> needs more memory.
>
> [1] 5837574: xen-netback: Fix grant ref resolution in RX path
>
> Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Thomas Graf <tgraf@suug.ch>
> Cc: Joe Perches <joe@perches.com>
> Cc: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: xen-devel@lists.xenproject.org
> ---
> v2: some bugfixes for pattern_to_packet, as my upcoming patch revealed some
>
>   net/core/pktgen.c |  139 ++++++++++++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 126 insertions(+), 13 deletions(-)
>
> diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> index 0304f98..253b83d 100644
> --- a/net/core/pktgen.c
> +++ b/net/core/pktgen.c
> @@ -201,6 +201,7 @@
>   #define F_QUEUE_MAP_CPU (1<<14)	/* queue map mirrors smp_processor_id() */
>   #define F_NODE          (1<<15)	/* Node memory alloc*/
>   #define F_UDPCSUM       (1<<16)	/* Include UDP checksum */
> +#define F_PATTERN       (1<<17)	/* Fill the payload with a pattern */
>   
>   /* Thread control flag bits */
>   #define T_STOP        (1<<0)	/* Stop run */
> @@ -255,7 +256,7 @@ struct pktgen_dev {
>   	int max_pkt_size;
>   	int pkt_overhead;	/* overhead for MPLS, VLANs, IPSEC etc */
>   	int nfrags;
> -	struct page *page;
> +	struct page *pages[MAX_SKB_FRAGS];
>   	u64 delay;		/* nano-seconds */
>   
>   	__u64 count;		/* Default No packets to send */
> @@ -1127,11 +1128,13 @@ static ssize_t pktgen_if_write(struct file *file,
>   		i += len;
>   
>   		if (node_possible(value)) {
> +			int j;
>   			pkt_dev->node = value;
>   			sprintf(pg_result, "OK: node=%d", pkt_dev->node);
> -			if (pkt_dev->page) {
> -				put_page(pkt_dev->page);
> -				pkt_dev->page = NULL;
> +			for (j = 0; j < MAX_SKB_FRAGS; ++j)
> +				if (pkt_dev->pages[j]) {
> +					put_page(pkt_dev->pages[j]);
> +					pkt_dev->pages[j] = NULL;
>   			}
>   		}
>   		else
> @@ -1242,6 +1245,12 @@ static ssize_t pktgen_if_write(struct file *file,
>   		else if (strcmp(f, "!UDPCSUM") == 0)
>   			pkt_dev->flags &= ~F_UDPCSUM;
>   
> +		else if (strcmp(f, "PATTERN") == 0)
> +			pkt_dev->flags |= F_PATTERN;
> +
> +		else if (strcmp(f, "!PATTERN") == 0)
> +			pkt_dev->flags &= ~F_PATTERN;
> +
>   		else {
>   			sprintf(pg_result,
>   				"Flag -:%s:- unknown\nAvailable flags, (prepend ! to un-set flag):\n%s",
> @@ -2623,17 +2632,98 @@ static inline __be16 build_tci(unsigned int id, unsigned int cfi,
>   	return htons(id | (cfi << 12) | (prio << 13));
>   }
>   
> +/* Max number of digits. The sizeof equals to log base 2^8 (UINT_MAX), multiply
> + * with 3 is a cheap, rounded up conversion to log10
> + */
> +#define UINT_MAX_DIGITS (3*sizeof(unsigned long)+1)
> +
> +/* Fill the buffer up with a pattern
> + * buf - pointer to buffer
> + * bufsize - size of the buffer
> + * start - starting value for the pattern
> + * incomplete - pointer to the offset inside the pattern, or 0 if none
> + *
> + * The pattern is a repetition of " %lu", a series of increasing numbers divided
> + * by space. The value of the number is "start" plus the size of the preceding
> + * space. E.g. if start is 1000, it starts like " 1000 1005 1010".
> + * If the last number doesn't fit, it gets truncated, and the number of leading
> + * characters is saved into incomplete. It can be passed then to the next call
> + * with the next buffer, and the pattern looks contiguous over scattered
> + * buffers.
> + * It returns "start" plus the offset of the byte after the last full
> + * pattern write.
> + */
> +static unsigned long pattern_to_packet(char *buf,
> +				       int bufsize,
> +				       unsigned long start,
> +				       unsigned int *incomplete)
> +{
> +	int len;
> +	/* Only used when the pattern doesn't align to the buffer */
> +	char temp[UINT_MAX_DIGITS];
> +
> +	if (*incomplete) {
> +		int copylen, offset = *incomplete;
> +
> +		len = snprintf(temp, sizeof(temp), " %lu", start);
> +		copylen = len - *incomplete;
> +		if (copylen > bufsize) {
> +			/* The continuation of this number couldn't fit here */
> +			copylen = bufsize;
> +			*incomplete += bufsize;
> +		} else {
> +			*incomplete = 0;
> +			start += len;
> +		}
> +		memcpy(buf, temp + offset, copylen);
> +		bufsize -= copylen;
> +		buf += copylen;
> +	}
> +
> +	while (bufsize > 0) {
> +		len = snprintf(buf, bufsize, " %lu", start);
> +		/* The last number doesn't fit, remember where it was truncated.
> +		 */
> +		if (len >= bufsize) {
> +			/* snprintf always add a trailing zero, but actually we
> +			 * need the last digit there
> +			 */
> +			len = snprintf(temp, sizeof(temp), " %lu", start);
> +			memcpy(buf, temp, bufsize);
> +			/* If the last number just fit without the trailing
> +			 * zero, the next buffer can continue from an increased
> +			 * offset.
> +			 */
> +			if (len == bufsize)
> +				start += len;
> +			*incomplete = bufsize;
> +			return start;
> +		}
> +		bufsize -= len;
> +		start += len;
> +		buf += len;
> +	}
> +	return start;
> +}
> +
>   static void pktgen_finalize_skb(struct pktgen_dev *pkt_dev, struct sk_buff *skb,
>   				int datalen)
>   {
>   	struct timeval timestamp;
>   	struct pktgen_hdr *pgh;
> +	unsigned long offset = 0;
> +	unsigned int incomplete = 0;
>   
>   	pgh = (struct pktgen_hdr *)skb_put(skb, sizeof(*pgh));
>   	datalen -= sizeof(*pgh);
>   
>   	if (pkt_dev->nfrags <= 0) {
> -		memset(skb_put(skb, datalen), 0, datalen);
> +		if (pkt_dev->flags & F_PATTERN)
> +			offset = pattern_to_packet(skb_put(skb, datalen),
> +						   datalen, offset,
> +						   &incomplete);
> +		else
> +			memset(skb_put(skb, datalen), 0, datalen);
>   	} else {
>   		int frags = pkt_dev->nfrags;
>   		int i, len;
> @@ -2644,7 +2734,12 @@ static void pktgen_finalize_skb(struct pktgen_dev *pkt_dev, struct sk_buff *skb,
>   			frags = MAX_SKB_FRAGS;
>   		len = datalen - frags * PAGE_SIZE;
>   		if (len > 0) {
> -			memset(skb_put(skb, len), 0, len);
> +			if (pkt_dev->flags & F_PATTERN)
> +				offset = pattern_to_packet(skb_put(skb, len),
> +							   len, offset,
> +							   &incomplete);
> +			else
> +				memset(skb_put(skb, len), 0, len);
>   			datalen = frags * PAGE_SIZE;
>   		}
>   
> @@ -2652,17 +2747,28 @@ static void pktgen_finalize_skb(struct pktgen_dev *pkt_dev, struct sk_buff *skb,
>   		frag_len = (datalen/frags) < PAGE_SIZE ?
>   			   (datalen/frags) : PAGE_SIZE;
>   		while (datalen > 0) {
> -			if (unlikely(!pkt_dev->page)) {
> +			int fragpage;
> +			gfp_t flags = GFP_KERNEL;
> +
> +			if (pkt_dev->flags & F_PATTERN) {
> +				fragpage = i;
> +			} else {
> +				fragpage = 0;
> +				flags |= __GFP_ZERO;
> +			}
> +
> +			if (unlikely(!pkt_dev->pages[fragpage])) {
>   				int node = numa_node_id();
>   
>   				if (pkt_dev->node >= 0 && (pkt_dev->flags & F_NODE))
>   					node = pkt_dev->node;
> -				pkt_dev->page = alloc_pages_node(node, GFP_KERNEL | __GFP_ZERO, 0);
> -				if (!pkt_dev->page)
> +				pkt_dev->pages[fragpage] = alloc_pages_node(node, flags, 0);
> +				if (!pkt_dev->pages[fragpage])
>   					break;
>   			}
> -			get_page(pkt_dev->page);
> -			skb_frag_set_page(skb, i, pkt_dev->page);
> +			get_page(pkt_dev->pages[fragpage]);
> +			skb_frag_set_page(skb, i, pkt_dev->pages[fragpage]);
> +
>   			skb_shinfo(skb)->frags[i].page_offset = 0;
>   			/*last fragment, fill rest of data*/
>   			if (i == (frags - 1))
> @@ -2671,6 +2777,10 @@ static void pktgen_finalize_skb(struct pktgen_dev *pkt_dev, struct sk_buff *skb,
>   			else
>   				skb_frag_size_set(&skb_shinfo(skb)->frags[i], frag_len);
>   			datalen -= skb_frag_size(&skb_shinfo(skb)->frags[i]);
> +			if (pkt_dev->flags & F_PATTERN)
> +				offset = pattern_to_packet(skb_frag_address(&skb_shinfo(skb)->frags[i]),
> +							   skb_frag_size(&skb_shinfo(skb)->frags[i]),
> +							   offset, &incomplete);
>   			skb->len += skb_frag_size(&skb_shinfo(skb)->frags[i]);
>   			skb->data_len += skb_frag_size(&skb_shinfo(skb)->frags[i]);
>   			i++;
> @@ -3685,6 +3795,8 @@ static void _rem_dev_from_if_list(struct pktgen_thread *t,
>   static int pktgen_remove_device(struct pktgen_thread *t,
>   				struct pktgen_dev *pkt_dev)
>   {
> +	int i;
> +
>   	pr_debug("remove_device pkt_dev=%p\n", pkt_dev);
>   
>   	if (pkt_dev->running) {
> @@ -3710,8 +3822,9 @@ static int pktgen_remove_device(struct pktgen_thread *t,
>   	free_SAs(pkt_dev);
>   #endif
>   	vfree(pkt_dev->flows);
> -	if (pkt_dev->page)
> -		put_page(pkt_dev->page);
> +	for (i = 0; i < MAX_SKB_FRAGS; ++i)
> +		if (pkt_dev->pages[i])
> +			put_page(pkt_dev->pages[i]);
>   	kfree(pkt_dev);
>   	return 0;
>   }

Re: [PATCH net-next] pktgen: Fill the payload optionally with a pattern

[David Miller]

From: Zoltan Kiss <zoltan.kiss@citrix.com>
Date: Tue, 24 Jun 2014 21:40:15 +0100

> Introduces a new flag called PATTERN, which puts a non-periodic, predicatble
> pattern into the payload. This was useful to reproduce an otherwise intermittent
> bug in xen-netback [1], where checksum checking doesn't help.
> The pattern is a repetition of " %lu", a series of increasing numbers divided by
> space. The value of the number is the size of the preceding payload area. E.g.
> " 1 3 5"..." 1000 1005 1010"
> If the pattern is used, every frag will have its own page, unlike before, so it
> needs more memory.
> 
> [1] 5837574: xen-netback: Fix grant ref resolution in RX path
> 
> Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>

You are changing the page allocation strategy regardless of the pattern
setting, this is undesirable.

It may be significantly faster to use the same page for all the frags,
and this is absolutely critical for pktgen usage where every nanosecond
of performance counts.

Re: [PATCH net-next] pktgen: Fill the payload optionally with a pattern

[Zoltan Kiss]

On 26/06/14 01:54, David Miller wrote:
> From: Zoltan Kiss <zoltan.kiss@citrix.com>
> Date: Tue, 24 Jun 2014 21:40:15 +0100
>
>> Introduces a new flag called PATTERN, which puts a non-periodic, predicatble
>> pattern into the payload. This was useful to reproduce an otherwise intermittent
>> bug in xen-netback [1], where checksum checking doesn't help.
>> The pattern is a repetition of " %lu", a series of increasing numbers divided by
>> space. The value of the number is the size of the preceding payload area. E.g.
>> " 1 3 5"..." 1000 1005 1010"
>> If the pattern is used, every frag will have its own page, unlike before, so it
>> needs more memory.
>>
>> [1] 5837574: xen-netback: Fix grant ref resolution in RX path
>>
>> Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
> You are changing the page allocation strategy regardless of the pattern
> setting, this is undesirable.
>
> It may be significantly faster to use the same page for all the frags,
> and this is absolutely critical for pktgen usage where every nanosecond
> of performance counts.
If the PATTERN flag is not used, it always using the pages[0] page, so 
it falls back to the original way.

Zoli

Re: [PATCH net-next] pktgen: Fill the payload optionally with a pattern

[David Miller]

From: Zoltan Kiss <zoltan.kiss@schaman.hu>
Date: Fri, 27 Jun 2014 10:01:23 +0100

> On 26/06/14 01:54, David Miller wrote:
>> From: Zoltan Kiss <zoltan.kiss@citrix.com>
>> Date: Tue, 24 Jun 2014 21:40:15 +0100
>>
>>> Introduces a new flag called PATTERN, which puts a non-periodic,
>>> predicatble
>>> pattern into the payload. This was useful to reproduce an otherwise
>>> intermittent
>>> bug in xen-netback [1], where checksum checking doesn't help.
>>> The pattern is a repetition of " %lu", a series of increasing numbers
>>> divided by
>>> space. The value of the number is the size of the preceding payload
>>> area. E.g.
>>> " 1 3 5"..." 1000 1005 1010"
>>> If the pattern is used, every frag will have its own page, unlike
>>> before, so it
>>> needs more memory.
>>>
>>> [1] 5837574: xen-netback: Fix grant ref resolution in RX path
>>>
>>> Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
>> You are changing the page allocation strategy regardless of the
>> pattern
>> setting, this is undesirable.
>>
>> It may be significantly faster to use the same page for all the frags,
>> and this is absolutely critical for pktgen usage where every
>> nanosecond
>> of performance counts.
> If the PATTERN flag is not used, it always using the pages[0] page, so
> it falls back to the original way.

That's now what I see.

If the size exceeds a page, the current code will use the same page over
and over again.

Your new code always increments 'i' and allocates a new page, regardless
of whether the PATTERN flag is set.

Or do I misread your changes?

Re: [PATCH net-next] pktgen: Fill the payload optionally with a pattern

[Zoltan Kiss]

On 27/06/14 20:30, David Miller wrote:
> From: Zoltan Kiss <zoltan.kiss@schaman.hu>
> Date: Fri, 27 Jun 2014 10:01:23 +0100
>
>> On 26/06/14 01:54, David Miller wrote:
>>> From: Zoltan Kiss <zoltan.kiss@citrix.com>
>>> Date: Tue, 24 Jun 2014 21:40:15 +0100
>>>
>>>> Introduces a new flag called PATTERN, which puts a non-periodic,
>>>> predicatble
>>>> pattern into the payload. This was useful to reproduce an otherwise
>>>> intermittent
>>>> bug in xen-netback [1], where checksum checking doesn't help.
>>>> The pattern is a repetition of " %lu", a series of increasing numbers
>>>> divided by
>>>> space. The value of the number is the size of the preceding payload
>>>> area. E.g.
>>>> " 1 3 5"..." 1000 1005 1010"
>>>> If the pattern is used, every frag will have its own page, unlike
>>>> before, so it
>>>> needs more memory.
>>>>
>>>> [1] 5837574: xen-netback: Fix grant ref resolution in RX path
>>>>
>>>> Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
>>> You are changing the page allocation strategy regardless of the
>>> pattern
>>> setting, this is undesirable.
>>>
>>> It may be significantly faster to use the same page for all the frags,
>>> and this is absolutely critical for pktgen usage where every
>>> nanosecond
>>> of performance counts.
>> If the PATTERN flag is not used, it always using the pages[0] page, so
>> it falls back to the original way.
>
> That's now what I see.
>
> If the size exceeds a page, the current code will use the same page over
> and over again.
>
> Your new code always increments 'i' and allocates a new page, regardless
> of whether the PATTERN flag is set.
>
> Or do I misread your changes?

My first patch uses fragpage to index the pages array, and it is always 
0, if the PATTERN frag were not specified. (i was incremented by the old 
code as well btw.)
The second one (see it in the other thread) adds individual frag size 
and offset, if that exceeds PAGE_SIZE, then it always reallocate the 
page, but I guess that's not a terrible thing, as the aim here is not 
performance testing, but finding bugs. But I can add a further checking 
to compare the allocation order of the current page with the required one.
Also, I think I should keep the __GFP_ZERO flags for every page 
allocation. Even if I fill up the frag with a pattern, it's better to 
have a deterministic content on the rest of the page, just in case some 
royal screwup leaks data from the rest of the page.
Also, turning off the PATTERN flag doesn't reallocate the pages, so the 
next packet will have the pattern from the previous instead of zeros.

Zoli