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.
diff --git sys/net/ifq.c sys/net/ifq.c
index 5221d013ec8..f27d3b736bd 100644
--- sys/net/ifq.c
+++ sys/net/ifq.c
@@ -251,39 +251,34 @@ ifq_destroy(struct ifqueue *ifq)
ml_purge(&ml);
}
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) {
ifq->ifq_len++;
ifq->ifq_packets++;
ifq->ifq_bytes += m->m_pkthdr.len;
if (ISSET(m->m_flags, M_MCAST))
ifq->ifq_mcasts++;
} else
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)
{
struct mbuf *m = NULL;
void *cookie;
