| Message ID | 20171113200838.6245.5586.stgit@john-Precision-Tower-5810 |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | lockless qdisc | expand |
On Mon, Nov 13, 2017 at 3:08 PM, John Fastabend <john.fastabend@gmail.com> wrote: > sch_direct_xmit() uses qdisc_qlen as a return value but all call sites > of the routine only check if it is zero or not. Simplify the logic so > that we don't need to return an actual queue length value. > > This introduces a case now where sch_direct_xmit would have returned > a qlen of zero but now it returns true. You mean the first case, when the likely(skb) branch failed? Can that return false, then? > However in this case all > call sites of sch_direct_xmit will implement a dequeue() and get > a null skb and abort. This trades tracking qlen in the hotpath for > an extra dequeue operation. Overall this seems to be good for > performance. > > Signed-off-by: John Fastabend <john.fastabend@gmail.com> > @@ -198,18 +198,19 @@ int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q, > > if (dev_xmit_complete(ret)) { > /* Driver sent out skb successfully or skb was consumed */ > - ret = qdisc_qlen(q); > + ret = true; > } else { > /* Driver returned NETDEV_TX_BUSY - requeue skb */ > if (unlikely(ret != NETDEV_TX_BUSY)) > net_warn_ratelimited("BUG %s code %d qlen %d\n", > dev->name, ret, q->q.qlen); > > - ret = dev_requeue_skb(skb, q); > + dev_requeue_skb(skb, q); > + ret = false; > } > > if (ret && netif_xmit_frozen_or_stopped(txq)) > - ret = 0; > + ret = false; > > return ret; > } This can now become if (!dev_queue_complete(ret)) { if (unlikely(ret != NETDEV_TX_BUSY)) ... dev_requeue_skb(skb, q); return false; } if (netif_xmit_frozen_or_stopped(txq)) return false; return true;
On Tue, Nov 14, 2017 at 7:11 PM, Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > On Mon, Nov 13, 2017 at 3:08 PM, John Fastabend > <john.fastabend@gmail.com> wrote: >> sch_direct_xmit() uses qdisc_qlen as a return value but all call sites >> of the routine only check if it is zero or not. Simplify the logic so >> that we don't need to return an actual queue length value. >> >> This introduces a case now where sch_direct_xmit would have returned >> a qlen of zero but now it returns true. > > You mean the first case, when the likely(skb) branch failed? > Can that return false, then? I misunderstood. __qdisc_run will just take one extra loop, since it will no longer break out of the loop on reading the last packet on the queue with qdisc_restart. That does have a subtle (but benign if rare) side effect of possibly calling __netif_schedule while there is nothing queued, if either the quota is exactly exhausted or need_resched is true.
On 11/14/2017 05:56 PM, Willem de Bruijn wrote: > On Tue, Nov 14, 2017 at 7:11 PM, Willem de Bruijn > <willemdebruijn.kernel@gmail.com> wrote: >> On Mon, Nov 13, 2017 at 3:08 PM, John Fastabend >> <john.fastabend@gmail.com> wrote: >>> sch_direct_xmit() uses qdisc_qlen as a return value but all call sites >>> of the routine only check if it is zero or not. Simplify the logic so >>> that we don't need to return an actual queue length value. >>> >>> This introduces a case now where sch_direct_xmit would have returned >>> a qlen of zero but now it returns true. >> >> You mean the first case, when the likely(skb) branch failed? >> Can that return false, then? > > I misunderstood. __qdisc_run will just take one extra loop, since it > will no longer break out of the loop on reading the last packet on > the queue with qdisc_restart. > > That does have a subtle (but benign if rare) side effect of possibly > calling __netif_schedule while there is nothing queued, if either > the quota is exactly exhausted or need_resched is true. > Right. Seems OK to me. Will also make code cleanup comment from intial email. .John
diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h index 4eea719..2404692 100644 --- a/include/net/pkt_sched.h +++ b/include/net/pkt_sched.h @@ -105,9 +105,9 @@ struct qdisc_rate_table *qdisc_get_rtab(struct tc_ratespec *r, void qdisc_put_rtab(struct qdisc_rate_table *tab); void qdisc_put_stab(struct qdisc_size_table *tab); void qdisc_warn_nonwc(const char *txt, struct Qdisc *qdisc); -int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q, - struct net_device *dev, struct netdev_queue *txq, - spinlock_t *root_lock, bool validate); +bool sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q, + struct net_device *dev, struct netdev_queue *txq, + spinlock_t *root_lock, bool validate); void __qdisc_run(struct Qdisc *q); diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c index ec757f6..d063f6b 100644 --- a/net/sched/sch_generic.c +++ b/net/sched/sch_generic.c @@ -164,12 +164,12 @@ static struct sk_buff *dequeue_skb(struct Qdisc *q, bool *validate, * only one CPU can execute this function. * * Returns to the caller: - * 0 - queue is empty or throttled. - * >0 - queue is not empty. + * false - hardware queue frozen backoff + * true - feel free to send more pkts */ -int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q, - struct net_device *dev, struct netdev_queue *txq, - spinlock_t *root_lock, bool validate) +bool sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q, + struct net_device *dev, struct netdev_queue *txq, + spinlock_t *root_lock, bool validate) { int ret = NETDEV_TX_BUSY; @@ -190,7 +190,7 @@ int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q, } else { if (root_lock) spin_lock(root_lock); - return qdisc_qlen(q); + return true; } if (root_lock) @@ -198,18 +198,19 @@ int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q, if (dev_xmit_complete(ret)) { /* Driver sent out skb successfully or skb was consumed */ - ret = qdisc_qlen(q); + ret = true; } else { /* Driver returned NETDEV_TX_BUSY - requeue skb */ if (unlikely(ret != NETDEV_TX_BUSY)) net_warn_ratelimited("BUG %s code %d qlen %d\n", dev->name, ret, q->q.qlen); - ret = dev_requeue_skb(skb, q); + dev_requeue_skb(skb, q); + ret = false; } if (ret && netif_xmit_frozen_or_stopped(txq)) - ret = 0; + ret = false; return ret; } @@ -233,7 +234,7 @@ int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q, * >0 - queue is not empty. * */ -static inline int qdisc_restart(struct Qdisc *q, int *packets) +static inline bool qdisc_restart(struct Qdisc *q, int *packets) { spinlock_t *root_lock = NULL; struct netdev_queue *txq; @@ -244,7 +245,7 @@ static inline int qdisc_restart(struct Qdisc *q, int *packets) /* Dequeue packet */ skb = dequeue_skb(q, &validate, packets); if (unlikely(!skb)) - return 0; + return false; if (!(q->flags & TCQ_F_NOLOCK)) root_lock = qdisc_lock(q);
sch_direct_xmit() uses qdisc_qlen as a return value but all call sites of the routine only check if it is zero or not. Simplify the logic so that we don't need to return an actual queue length value. This introduces a case now where sch_direct_xmit would have returned a qlen of zero but now it returns true. However in this case all call sites of sch_direct_xmit will implement a dequeue() and get a null skb and abort. This trades tracking qlen in the hotpath for an extra dequeue operation. Overall this seems to be good for performance. Signed-off-by: John Fastabend <john.fastabend@gmail.com> --- 0 files changed