On 06/03/17(Mon) 23:13, Mike Belopuhov wrote:
> 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.
I'm puzzled, if dm is not NULL we dropped a packet, no? In that
case the length of the queue did not change. So ``ifq_len''
shouldn't be updated, right?
> ---
> 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
>
