On Thu, Mar 02, 2017 at 14:23 +0100, Mike Belopuhov wrote:
> On Thu, Mar 02, 2017 at 10:35 +1000, David Gwynne wrote:
> > the current code has been very careful not to free an mbuf while
> > holding the ifq mutex. i would prefer to keep it that way.
> >
> > the least worst way to do that would be to return the mbuf to be
> > dropped for ifq_enqueue to free. this is complicated because of the
> > semantics that ifq_enqueue_try provides, but nothing uses that so
> > we can get rid of it to support this.
> >
> > the diff below makes the ifq enq op return an mbuf to be freed, and
> > gets rid of ifq_enqueue_try. that in turn should let you return
> > this mbuf here rather than free it directly.
> >
>
> The diff is OK by me provided that a fix like the one below is
> included. We only need to return ENOBUFS when we've dropped
> the very packet we were trying to enqueue since the error is
> propagated up the stack to the userland.
>
Correction: we should do the "ifq->ifq_len++" block when we've
successfully enqueued the packet we had. dm can refer to some
other one, so technically queue stats need to be adjusted.
---
sys/net/ifq.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git sys/net/ifq.c sys/net/ifq.c
index 5221d013ec8..bfd5cfd6297 100644
--- sys/net/ifq.c
+++ sys/net/ifq.c
@@ -254,33 +254,29 @@ ifq_destroy(struct ifqueue *ifq)
int
ifq_enqueue(struct ifqueue *ifq, struct mbuf *m)
{
struct mbuf *dm;
- int rv;
mtx_enter(&ifq->ifq_mtx);
dm = ifq->ifq_ops->ifqop_enq(ifq, m);
- if (dm == NULL) {
+ if (dm == NULL || dm != m) {
ifq->ifq_len++;
ifq->ifq_packets++;
ifq->ifq_bytes += m->m_pkthdr.len;
if (ISSET(m->m_flags, M_MCAST))
ifq->ifq_mcasts++;
- } else
+ }
+ if (dm != NULL)
ifq->ifq_qdrops++;
mtx_leave(&ifq->ifq_mtx);
- if (dm == NULL)
- rv = 0;
- else {
+ if (dm != NULL)
m_freem(dm);
- rv = ENOBUFS;
- }
- return (rv);
+ return (dm == m ? ENOBUFS : 0);
}
struct mbuf *
ifq_deq_begin(struct ifqueue *ifq)
{
--
2.12.0
