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.
>
> 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;
>
I've rebased everything on top of this: http://gir.theapt.org/~mike/ifq-priq/
So I guess I'm ready when you are.
