| Message ID | 568FC9F8.8040201@mojatatu.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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);