| Message ID | 20160629194731.14745.20562.stgit@john-Precision-Tower-5810 |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On 16-06-29 03:47 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> > --- > net/core/pktgen.c | 42 ++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 40 insertions(+), 2 deletions(-) > > diff --git a/net/core/pktgen.c b/net/core/pktgen.c > index f74ab9c..4b3d467 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: "); > > @@ -1142,8 +1145,10 @@ static ssize_t pktgen_if_write(struct file *file, > return len; > > i += len; > - if ((value > 1) && (pkt_dev->xmit_mode == M_START_XMIT) && > - (!(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING))) > + if ((value > 1) && > + ((pkt_dev->xmit_mode == M_QUEUE_XMIT) || > + ((pkt_dev->xmit_mode == M_START_XMIT) && > + (!(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING))))) > return -ENOTSUPP; > pkt_dev->burst = value < 1 ? 1 : value; > sprintf(pg_result, "OK: burst=%d", pkt_dev->burst); > @@ -1198,6 +1203,9 @@ 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; > + pkt_dev->last_ok = 1; > } else { > sprintf(pg_result, > "xmit_mode -:%s:- unknown\nAvailable modes: %s", > @@ -3434,6 +3442,36 @@ 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) { > + local_bh_disable(); > + atomic_add(burst, &pkt_dev->skb->users); > + > + ret = dev_queue_xmit(pkt_dev->skb); > + switch (ret) { > + case NET_XMIT_SUCCESS: > + pkt_dev->sofar++; > + pkt_dev->seq_num++; > + pkt_dev->tx_bytes += pkt_dev->last_pkt_size; > + break; > + case NET_XMIT_DROP: > + case NET_XMIT_CN: > + /* These are all valid return codes for a qdisc but > + * indicate packets are being dropped or will likely > + * be dropped soon. > + */ > + 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. > + */ > + default: > + pkt_dev->errors++; > + net_info_ratelimited("%s xmit error: %d\n", > + pkt_dev->odevname, ret); > + break; > + } > + goto out; > } > > txq = skb_get_tx_queue(odev, pkt_dev->skb); > Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> In travel mode, dont have much cycles right now - but can you review again: http://www.spinics.net/lists/netdev/msg359545.html I think you should disallow clone for example and i wasnt sure if you covered all error scenarios etc. cheers, jamal
On 16-06-30 03:21 AM, Jamal Hadi Salim wrote: > On 16-06-29 03:47 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> >> --- [...] > > Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> > > In travel mode, dont have much cycles right now - but can you review > again: > http://www.spinics.net/lists/netdev/msg359545.html > I think you should disallow clone for example and i wasnt sure if you > covered all error scenarios etc. > Taking a look at the link couple differences exist. First the patch linked does a 'netif_xmit_frozen_or_drv_stopped(txq)' check but this really shouldn't be needed it is handled by the sch_direct_xmit() logic in ./net/sched Also in this patch I made it way more conservative on when to back off then my original patch and now its closer to the one linked except I also back off with return code NET_XMIT_CN. As for clones what is the concern exactly? We allow them through the ingress pktgen mode that can hit classifiers and I don't see any issues testing with them. .John > cheers, > jamal
On 16-06-30 12:53 PM, John Fastabend wrote: > On 16-06-30 03:21 AM, Jamal Hadi Salim wrote: >> On 16-06-29 03:47 PM, John Fastabend wrote: > > Taking a look at the link couple differences exist. First the patch > linked does a 'netif_xmit_frozen_or_drv_stopped(txq)' check but this > really shouldn't be needed it is handled by the sch_direct_xmit() > logic in ./net/sched > > Also in this patch I made it way more conservative on when to back > off then my original patch and now its closer to the one linked except > I also back off with return code NET_XMIT_CN. > > As for clones what is the concern exactly? We allow them through the > ingress pktgen mode that can hit classifiers and I don't see any issues > testing with them. > I sent you that because we discussed back then and you thought there was something different and useful. I dont remember why it was important to avoid clones - maybe i had a bug. I thought we discussed it (I will try to dig into the machine used for testing). Anyways, doesnt matter if you tested and it works fine. cheers, jamal
On 16-07-01 05:56 AM, Jamal Hadi Salim wrote: > On 16-06-30 12:53 PM, John Fastabend wrote: >> On 16-06-30 03:21 AM, Jamal Hadi Salim wrote: >>> On 16-06-29 03:47 PM, John Fastabend wrote: > >> >> Taking a look at the link couple differences exist. First the patch >> linked does a 'netif_xmit_frozen_or_drv_stopped(txq)' check but this >> really shouldn't be needed it is handled by the sch_direct_xmit() >> logic in ./net/sched >> >> Also in this patch I made it way more conservative on when to back >> off then my original patch and now its closer to the one linked except >> I also back off with return code NET_XMIT_CN. >> >> As for clones what is the concern exactly? We allow them through the >> ingress pktgen mode that can hit classifiers and I don't see any issues >> testing with them. >> > > I sent you that because we discussed back then and you thought > there was something different and useful. > I dont remember why it was important to avoid clones - maybe i had a > bug. I thought we discussed it (I will try to dig into the machine > used for testing). > Anyways, doesnt matter if you tested and it works fine. Ah I was originally ignoring more of the error cases and continuing to try and enqueue packets into the qdisc even though it was returning error codes. You suggested we should abort in the error case and I adopted your idea in this patch. .John > > cheers, > jamal >
diff --git a/net/core/pktgen.c b/net/core/pktgen.c index f74ab9c..4b3d467 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: "); @@ -1142,8 +1145,10 @@ static ssize_t pktgen_if_write(struct file *file, return len; i += len; - if ((value > 1) && (pkt_dev->xmit_mode == M_START_XMIT) && - (!(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING))) + if ((value > 1) && + ((pkt_dev->xmit_mode == M_QUEUE_XMIT) || + ((pkt_dev->xmit_mode == M_START_XMIT) && + (!(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING))))) return -ENOTSUPP; pkt_dev->burst = value < 1 ? 1 : value; sprintf(pg_result, "OK: burst=%d", pkt_dev->burst); @@ -1198,6 +1203,9 @@ 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; + pkt_dev->last_ok = 1; } else { sprintf(pg_result, "xmit_mode -:%s:- unknown\nAvailable modes: %s", @@ -3434,6 +3442,36 @@ 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) { + local_bh_disable(); + atomic_add(burst, &pkt_dev->skb->users); + + ret = dev_queue_xmit(pkt_dev->skb); + switch (ret) { + case NET_XMIT_SUCCESS: + pkt_dev->sofar++; + pkt_dev->seq_num++; + pkt_dev->tx_bytes += pkt_dev->last_pkt_size; + break; + case NET_XMIT_DROP: + case NET_XMIT_CN: + /* These are all valid return codes for a qdisc but + * indicate packets are being dropped or will likely + * be dropped soon. + */ + 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. + */ + default: + pkt_dev->errors++; + net_info_ratelimited("%s xmit error: %d\n", + pkt_dev->odevname, ret); + break; + } + goto out; } txq = skb_get_tx_queue(odev, pkt_dev->skb);
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> --- net/core/pktgen.c | 42 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-)