On 7 March 2017 at 10:13, Martin Pieuchot <[email protected]> wrote: > 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? >
dlg has committed a cleaner version, but you're correct. the queue length shouldn't be updated. that was an oversight.
