[PATCH] net: pktgen: support injecting packets for qdisc testing

[PATCH] net: pktgen: support injecting packets for qdisc testing

[John Fastabend]

Add another xmit_mode to pktgen to allow testing xmit functionality
of qdiscs. The new mode "queue_xmit" injects packets at
__dev_queue_xmit() so that qdisc is called.

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---
 Documentation/networking/pktgen.txt |    2 +-
 net/core/pktgen.c                   |   43 +++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/Documentation/networking/pktgen.txt b/Documentation/networking/pktgen.txt
index f4be85e..aca62e2 100644
--- a/Documentation/networking/pktgen.txt
+++ b/Documentation/networking/pktgen.txt
@@ -365,7 +365,7 @@ flowlen
 rate
 ratep
 
-xmit_mode <start_xmit|netif_receive>
+xmit_mode <start_xmit|netif_receive|queue_xmit>
 
 vlan_cfi
 vlan_id
diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 2be1444..a86bda5 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -213,6 +213,7 @@
 /* Xmit modes */
 #define M_START_XMIT		0	/* Default normal TX */
 #define M_NETIF_RECEIVE 	1	/* Inject packets into stack */
+#define M_QUEUE_XMIT		2	/* Inject packet into qdisc */
 
 /* If lock -- protects updating of if_list */
 #define   if_lock(t)           spin_lock(&(t->if_lock));
@@ -626,6 +627,8 @@ static int pktgen_if_show(struct seq_file *seq, void *v)
 
 	if (pkt_dev->xmit_mode == M_NETIF_RECEIVE)
 		seq_puts(seq, "     xmit_mode: netif_receive\n");
+	else if (pkt_dev->xmit_mode == M_QUEUE_XMIT)
+		seq_puts(seq, "     xmit_mode: xmit_queue\n");
 
 	seq_puts(seq, "     Flags: ");
 
@@ -1198,6 +1201,8 @@ static ssize_t pktgen_if_write(struct file *file,
 			 * at module loading time
 			 */
 			pkt_dev->clone_skb = 0;
+		} else if (strcmp(f, "queue_xmit") == 0) {
+			pkt_dev->xmit_mode = M_QUEUE_XMIT;
 		} else {
 			sprintf(pg_result,
 				"xmit_mode -:%s:- unknown\nAvailable modes: %s",
@@ -3432,6 +3437,44 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
 #endif
 		} while (--burst > 0);
 		goto out; /* Skips xmit_mode M_START_XMIT */
+	} else if (pkt_dev->xmit_mode == M_QUEUE_XMIT) {
+		atomic_add(burst, &pkt_dev->skb->users);
+		local_bh_disable();
+
+		do {
+			ret = dev_queue_xmit(pkt_dev->skb);
+			switch (ret) {
+			case NET_XMIT_SUCCESS:
+			case NET_XMIT_DROP:
+			case NET_XMIT_CN:
+			case NET_XMIT_POLICED:
+			/* These are all valid return codes for a qdisc so
+			 * unlike start_xmit report this as successful
+			 * transactions. I expect testers will be also be
+			 * reviewing qdisc stats for more details.
+			 */
+				pkt_dev->last_ok = 1;
+				pkt_dev->sofar++;
+				pkt_dev->seq_num++;
+				pkt_dev->tx_bytes += pkt_dev->last_pkt_size;
+				break;
+			case NETDEV_TX_LOCKED:
+			case NETDEV_TX_BUSY:
+			/* qdisc may call dev_hard_start_xmit directly in cases
+			 * where no queues exist e.g. loopback device, virtual
+			 * devices, etc. In this case we need to handle
+			 * NETDEV_TX_ codes.
+			 */
+				pkt_dev->errors++;
+				break;
+			default:
+				net_info_ratelimited("%s xmit error: %d\n",
+						     pkt_dev->odevname, ret);
+				pkt_dev->errors++;
+				break;
+			}
+		} while (--burst > 0);
+		goto out;
 	}
 
 	txq = skb_get_tx_queue(odev, pkt_dev->skb);

Re: [PATCH] net: pktgen: support injecting packets for qdisc testing

[John Fastabend]

On 16-01-07 06:09 PM, John Fastabend wrote:
> Add another xmit_mode to pktgen to allow testing xmit functionality
> of qdiscs. The new mode "queue_xmit" injects packets at
> __dev_queue_xmit() so that qdisc is called.
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> ---
>  Documentation/networking/pktgen.txt |    2 +-
>  net/core/pktgen.c                   |   43 +++++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+), 1 deletion(-)
> 

Hi Jamal,

per your comment about using pktgen to test qdiscs here is the
patch I've been using most the day which has been working well.
I'm guessing this is more or less what you had in mind.

Thanks,
John

Re: [PATCH] net: pktgen: support injecting packets for qdisc testing

[Jamal Hadi Salim]

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

On 16-01-07 09:28 PM, John Fastabend wrote:
>
> Hi Jamal,
>
> per your comment about using pktgen to test qdiscs here is the
> patch I've been using most the day which has been working well.
> I'm guessing this is more or less what you had in mind.
>

I have attached the one i used over the holidays - see if there are
any differences(sorry dont have much time right now).

cheers,
jamal

[-- Attachment #2: pktgen-patch --]
[-- Type: text/plain, Size: 4394 bytes --]

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index de8d5cc..30774f1 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -212,7 +212,8 @@
 
 /* Xmit modes */
 #define M_START_XMIT		0	/* Default normal TX */
-#define M_NETIF_RECEIVE 	1	/* Inject packets into stack */
+#define M_NETIF_RECEIVE		1	/* Inject packets into stack */
+#define M_NETIF_QUEUE_XMIT	2	/* Inject packets into qdisc */
 
 /* If lock -- protects updating of if_list */
 #define   if_lock(t)           spin_lock(&(t->if_lock));
@@ -616,7 +617,8 @@ static int pktgen_if_show(struct seq_file *seq, void *v)
 		seq_printf(seq, "     tos: 0x%02x\n", pkt_dev->tos);
 
 	if (pkt_dev->traffic_class)
-		seq_printf(seq, "     traffic_class: 0x%02x\n", pkt_dev->traffic_class);
+		seq_printf(seq, "     traffic_class: 0x%02x\n",
+			   pkt_dev->traffic_class);
 
 	if (pkt_dev->burst > 1)
 		seq_printf(seq, "     burst: %d\n", pkt_dev->burst);
@@ -624,8 +626,12 @@ static int pktgen_if_show(struct seq_file *seq, void *v)
 	if (pkt_dev->node >= 0)
 		seq_printf(seq, "     node: %d\n", pkt_dev->node);
 
+	if (pkt_dev->xmit_mode == M_START_XMIT)
+		seq_puts(seq, "     xmit_mode: qdisc_bypass\n");
 	if (pkt_dev->xmit_mode == M_NETIF_RECEIVE)
 		seq_puts(seq, "     xmit_mode: netif_receive\n");
+	if (pkt_dev->xmit_mode == M_NETIF_QUEUE_XMIT)
+		seq_puts(seq, "     xmit_mode: qdisc_xmit\n");
 
 	seq_puts(seq, "     Flags: ");
 
@@ -1198,6 +1204,22 @@ static ssize_t pktgen_if_write(struct file *file,
 			 * at module loading time
 			 */
 			pkt_dev->clone_skb = 0;
+		} else if (strcmp(f, "qdisc_xmit") == 0) {
+			/* clone_skb set earlier, not supported in this mode */
+			if (pkt_dev->clone_skb > 0)
+				return -ENOTSUPP;
+
+			pkt_dev->xmit_mode = M_NETIF_QUEUE_XMIT;
+
+			/* make sure new packet is allocated every time
+			 * pktgen_xmit() is called
+			 */
+			pkt_dev->last_ok = 1;
+
+			/* override clone_skb if user passed default value
+			 * at module loading time
+			 */
+			pkt_dev->clone_skb = 0;
 		} else {
 			sprintf(pg_result,
 				"xmit_mode -:%s:- unknown\nAvailable modes: %s",
@@ -3361,6 +3383,23 @@ static void pktgen_wait_for_skb(struct pktgen_dev *pkt_dev)
 	pkt_dev->idle_acc += ktime_to_ns(ktime_sub(ktime_get(), idle_start));
 }
 
+static inline netdev_tx_t netdevq_start_xmit(struct sk_buff *skb,
+					    struct net_device *dev,
+					    struct netdev_queue *txq, bool more)
+{
+	const struct net_device_ops *ops = dev->netdev_ops;
+	int rc;
+
+	skb->xmit_more = more ? 1 : 0;
+	skb->dev = dev;
+	rc = dev_queue_xmit(skb);
+	if (likely(rc == NET_XMIT_SUCCESS || rc == NET_XMIT_CN)) {
+		txq_trans_update(txq);
+	}
+
+	return rc;
+}
+
 static void pktgen_xmit(struct pktgen_dev *pkt_dev)
 {
 	unsigned int burst = ACCESS_ONCE(pkt_dev->burst);
@@ -3432,8 +3471,54 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
 #endif
 		} while (--burst > 0);
 		goto out; /* Skips xmit_mode M_START_XMIT */
+	} else if (pkt_dev->xmit_mode == M_NETIF_QUEUE_XMIT) {
+
+		txq = skb_get_tx_queue(odev, pkt_dev->skb);
+		local_bh_disable();
+
+		if (unlikely(netif_xmit_frozen_or_drv_stopped(txq))) {
+			ret = NETDEV_TX_BUSY;
+			pkt_dev->last_ok = 0;
+			goto out;
+		}
+		atomic_add(burst, &pkt_dev->skb->users);
+
+qxmit_more:
+		ret = netdevq_start_xmit(pkt_dev->skb, odev, txq, --burst > 0);
+
+		switch (ret) {
+		case NETDEV_TX_OK:
+			pkt_dev->last_ok = 1;
+			pkt_dev->sofar++;
+			pkt_dev->seq_num++;
+			pkt_dev->tx_bytes += pkt_dev->last_pkt_size;
+			if (burst > 0 && !netif_xmit_frozen_or_drv_stopped(txq))
+				goto qxmit_more;
+			break;
+		case NET_XMIT_DROP:
+		case NET_XMIT_CN:
+		case NET_XMIT_POLICED:
+			/* skb has been consumed */
+			pkt_dev->errors++;
+			break;
+		default: /* Drivers are not supposed to return other values! */
+			net_info_ratelimited("%s xmit error: %d\n",
+					     pkt_dev->odevname, ret);
+			pkt_dev->errors++;
+			/* fallthru */
+		case NETDEV_TX_LOCKED:
+		case NETDEV_TX_BUSY:
+			/* Retry it next time */
+			atomic_dec(&(pkt_dev->skb->users));
+			pkt_dev->last_ok = 0;
+		}
+		if (unlikely(burst))
+			atomic_sub(burst, &pkt_dev->skb->users);
+
+		goto out; /* Skips xmit_mode M_START_XMIT */
 	}
 
+	/*M_START_XMIT*/
 	txq = skb_get_tx_queue(odev, pkt_dev->skb);
 
 	local_bh_disable();
@@ -3478,6 +3563,7 @@ xmit_more:
 	}
 	if (unlikely(burst))
 		atomic_sub(burst, &pkt_dev->skb->users);
+
 unlock:
 	HARD_TX_UNLOCK(odev, txq);
 

Re: [PATCH] net: pktgen: support injecting packets for qdisc testing

[John Fastabend]

On 16-01-08 06:38 AM, Jamal Hadi Salim wrote:
> On 16-01-07 09:28 PM, John Fastabend wrote:
>>
>> Hi Jamal,
>>
>> per your comment about using pktgen to test qdiscs here is the
>> patch I've been using most the day which has been working well.
>> I'm guessing this is more or less what you had in mind.
>>
> 
> I have attached the one i used over the holidays - see if there are
> any differences(sorry dont have much time right now).
> 
> cheers,
> jamal

Close except I don't back off the qdisc when I get
xmit_{drop|cn|policed} return codes because I'm trying to
stress the qdisc.

Also I don't see the point to doing the skb_get_tx_queue and
txq_trans_update() this should be called by dev_queue_xmit
anyways.

If its all the same to you I would just assume have my version in
pktgen and later if its useful for you add an option to back-off
when the qdisc throws an error.

Thanks,
John

Re: [PATCH] net: pktgen: support injecting packets for qdisc testing

[John Fastabend]

On 16-01-07 06:09 PM, John Fastabend wrote:
> Add another xmit_mode to pktgen to allow testing xmit functionality
> of qdiscs. The new mode "queue_xmit" injects packets at
> __dev_queue_xmit() so that qdisc is called.
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> ---


[...]

>  		} while (--burst > 0);
>  		goto out; /* Skips xmit_mode M_START_XMIT */
> +	} else if (pkt_dev->xmit_mode == M_QUEUE_XMIT) {
> +		atomic_add(burst, &pkt_dev->skb->users);
> +		local_bh_disable();
> +
> +		do {
> +			ret = dev_queue_xmit(pkt_dev->skb);
> +			switch (ret) {
> +			case NET_XMIT_SUCCESS:
> +			case NET_XMIT_DROP:
> +			case NET_XMIT_CN:
> +			case NET_XMIT_POLICED:
> +			/* These are all valid return codes for a qdisc so
> +			 * unlike start_xmit report this as successful
> +			 * transactions. I expect testers will be also be
> +			 * reviewing qdisc stats for more details.
> +			 */
> +				pkt_dev->last_ok = 1;
> +				pkt_dev->sofar++;
> +				pkt_dev->seq_num++;
> +				pkt_dev->tx_bytes += pkt_dev->last_pkt_size;
> +				break;
> +			case NETDEV_TX_LOCKED:
> +			case NETDEV_TX_BUSY:
> +			/* qdisc may call dev_hard_start_xmit directly in cases
> +			 * where no queues exist e.g. loopback device, virtual
> +			 * devices, etc. In this case we need to handle
> +			 * NETDEV_TX_ codes.
> +			 */
> +				pkt_dev->errors++;
> +				break;
> +			default:
> +				net_info_ratelimited("%s xmit error: %d\n",
> +						     pkt_dev->odevname, ret);
> +				pkt_dev->errors++;
> +				break;
> +			}
> +		} while (--burst > 0);


oops missed the case where burst != 0 here and we need to do an
atomic_sub on skb->users.

.John

Re: [PATCH] net: pktgen: support injecting packets for qdisc testing

[John Fastabend]

On 16-01-08 07:28 AM, John Fastabend wrote:
> On 16-01-07 06:09 PM, John Fastabend wrote:
>> Add another xmit_mode to pktgen to allow testing xmit functionality
>> of qdiscs. The new mode "queue_xmit" injects packets at
>> __dev_queue_xmit() so that qdisc is called.
>>
>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>> ---
> 
> 
> [...]
> 
>>  		} while (--burst > 0);
>>  		goto out; /* Skips xmit_mode M_START_XMIT */
>> +	} else if (pkt_dev->xmit_mode == M_QUEUE_XMIT) {
>> +		atomic_add(burst, &pkt_dev->skb->users);
>> +		local_bh_disable();
>> +
>> +		do {
>> +			ret = dev_queue_xmit(pkt_dev->skb);
>> +			switch (ret) {
>> +			case NET_XMIT_SUCCESS:
>> +			case NET_XMIT_DROP:
>> +			case NET_XMIT_CN:
>> +			case NET_XMIT_POLICED:
>> +			/* These are all valid return codes for a qdisc so
>> +			 * unlike start_xmit report this as successful
>> +			 * transactions. I expect testers will be also be
>> +			 * reviewing qdisc stats for more details.
>> +			 */
>> +				pkt_dev->last_ok = 1;
>> +				pkt_dev->sofar++;
>> +				pkt_dev->seq_num++;
>> +				pkt_dev->tx_bytes += pkt_dev->last_pkt_size;
>> +				break;
>> +			case NETDEV_TX_LOCKED:
>> +			case NETDEV_TX_BUSY:
>> +			/* qdisc may call dev_hard_start_xmit directly in cases
>> +			 * where no queues exist e.g. loopback device, virtual
>> +			 * devices, etc. In this case we need to handle
>> +			 * NETDEV_TX_ codes.
>> +			 */
>> +				pkt_dev->errors++;
>> +				break;
>> +			default:
>> +				net_info_ratelimited("%s xmit error: %d\n",
>> +						     pkt_dev->odevname, ret);
>> +				pkt_dev->errors++;
>> +				break;
>> +			}
>> +		} while (--burst > 0);
> 
> 
> oops missed the case where burst != 0 here and we need to do an
> atomic_sub on skb->users.
> 
> .John
> 

Also I'll respin this with a change so xmit_{drop|cn|policed} are not
counted as tx_bytes and pkts sent (idea from Jamal). However I won't
abort the burst because I do like this stressing the entry point of my
qdisc.

.John

Re: [PATCH] net: pktgen: support injecting packets for qdisc testing

[Eric Dumazet]

On Fri, 2016-01-08 at 07:38 -0800, John Fastabend wrote:

> Also I'll respin this with a change so xmit_{drop|cn|policed} are not
> counted as tx_bytes and pkts sent (idea from Jamal). However I won't
> abort the burst because I do like this stressing the entry point of my
> qdisc.

This reminds me that bonding/team drivers always return NETDEV_TX_OK
status, even if the chosen slave could not queue or send the packet.

Re: [PATCH] net: pktgen: support injecting packets for qdisc testing

[John Fastabend]

On 16-01-08 01:36 PM, Eric Dumazet wrote:
> On Fri, 2016-01-08 at 07:38 -0800, John Fastabend wrote:
> 
>> Also I'll respin this with a change so xmit_{drop|cn|policed} are not
>> counted as tx_bytes and pkts sent (idea from Jamal). However I won't
>> abort the burst because I do like this stressing the entry point of my
>> qdisc.
> 
> This reminds me that bonding/team drivers always return NETDEV_TX_OK
> status, even if the chosen slave could not queue or send the packet.
> 
> 
> 

hmm this could potentially break protocols that are handling the
return codes like fcoe which puts a packet back on its queue if it
can't be sent.

.John

Re: [PATCH] net: pktgen: support injecting packets for qdisc testing

[John Fastabend]

On 16-01-08 07:24 AM, John Fastabend wrote:
> On 16-01-08 06:38 AM, Jamal Hadi Salim wrote:
>> On 16-01-07 09:28 PM, John Fastabend wrote:
>>>
>>> Hi Jamal,
>>>
>>> per your comment about using pktgen to test qdiscs here is the
>>> patch I've been using most the day which has been working well.
>>> I'm guessing this is more or less what you had in mind.
>>>
>>
>> I have attached the one i used over the holidays - see if there are
>> any differences(sorry dont have much time right now).
>>
>> cheers,
>> jamal
> 
> Close except I don't back off the qdisc when I get
> xmit_{drop|cn|policed} return codes because I'm trying to
> stress the qdisc.
> 
> Also I don't see the point to doing the skb_get_tx_queue and
> txq_trans_update() this should be called by dev_queue_xmit
> anyways.
> 
> If its all the same to you I would just assume have my version in
> pktgen and later if its useful for you add an option to back-off
> when the qdisc throws an error.
> 
> Thanks,
> John
> 
> 

And burst > 1 will cause a panic as well when injecting the packet
into dev_queue_xmit(). It seems skb->users > 1 which is what the
burst value is tuning in pktgen will stop the skb from being free'd
but the stack doesn't check skb_shared().

.John

Re: [PATCH] net: pktgen: support injecting packets for qdisc testing

[Jamal Hadi Salim]

On 16-01-08 10:24 AM, John Fastabend wrote:
> On 16-01-08 06:38 AM, Jamal Hadi Salim wrote:
>> On 16-01-07 09:28 PM, John Fastabend wrote:
>>>
>>> Hi Jamal,
>>>
>>> per your comment about using pktgen to test qdiscs here is the
>>> patch I've been using most the day which has been working well.
>>> I'm guessing this is more or less what you had in mind.
>>>
>>
>> I have attached the one i used over the holidays - see if there are
>> any differences(sorry dont have much time right now).
>>
>> cheers,
>> jamal
>
> Close except I don't back off the qdisc when I get
> xmit_{drop|cn|policed} return codes because I'm trying to
> stress the qdisc.
>

In that case we need two modes. I think it is important to stress
the qdisc backoff. It doesnt make sense to send packets that will
be dropped. You said you were testing over vlan - how were you
enforcing no backoff?

> Also I don't see the point to doing the skb_get_tx_queue and
> txq_trans_update() this should be called by dev_queue_xmit
> anyways.
>

Yes, those could go away. They are leftovers of another experiment.


> If its all the same to you I would just assume have my version in
> pktgen and later if its useful for you add an option to back-off
> when the qdisc throws an error.


Refer to above. If you can fix it so we can do either i will ACK it.

cheers,
jamal

Re: [PATCH] net: pktgen: support injecting packets for qdisc testing

[John Fastabend]

On 16-01-10 06:09 AM, Jamal Hadi Salim wrote:
> On 16-01-08 10:24 AM, John Fastabend wrote:
>> On 16-01-08 06:38 AM, Jamal Hadi Salim wrote:
>>> On 16-01-07 09:28 PM, John Fastabend wrote:
>>>>
>>>> Hi Jamal,
>>>>
>>>> per your comment about using pktgen to test qdiscs here is the
>>>> patch I've been using most the day which has been working well.
>>>> I'm guessing this is more or less what you had in mind.
>>>>
>>>
>>> I have attached the one i used over the holidays - see if there are
>>> any differences(sorry dont have much time right now).
>>>
>>> cheers,
>>> jamal
>>
>> Close except I don't back off the qdisc when I get
>> xmit_{drop|cn|policed} return codes because I'm trying to
>> stress the qdisc.
>>
> 
> In that case we need two modes. I think it is important to stress
> the qdisc backoff. It doesnt make sense to send packets that will
> be dropped. You said you were testing over vlan - how were you
> enforcing no backoff?

it was aborting so I wasn't able to do this with vlans. It is
interesting for example with pfifo_tail_enqueue where we return
NET_XMIT_CN but still enqueue the packet. If only to test the
qdisc, agreed though "real" users should backoff.

> 
>> Also I don't see the point to doing the skb_get_tx_queue and
>> txq_trans_update() this should be called by dev_queue_xmit
>> anyways.
>>
> 
> Yes, those could go away. They are leftovers of another experiment.
> 
> 
>> If its all the same to you I would just assume have my version in
>> pktgen and later if its useful for you add an option to back-off
>> when the qdisc throws an error.
> 
> 
> Refer to above. If you can fix it so we can do either i will ACK it.

I'll send a Jamal approved version when net-next opens again ;)

Thanks!
John