Date: Mon, 19 Sep 2005 08:10:14 GMT From: Ruslan Ermilov <ru@freebsd.org> To: freebsd-bugs@FreeBSD.org Subject: Re: kern/86306: [patch] if_em.c locks up while trying to send a highly fragmented packet Message-ID: <200509190810.j8J8AE5P096775@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/86306; it has been noted by GNATS. From: Ruslan Ermilov <ru@freebsd.org> To: Dmitrij Tejblum <tejblum@yandex-team.ru> Cc: FreeBSD-gnats-submit@freebsd.org Subject: Re: kern/86306: [patch] if_em.c locks up while trying to send a highly fragmented packet Date: Mon, 19 Sep 2005 11:09:46 +0300 On Mon, Sep 19, 2005 at 11:38:47AM +0400, Dmitrij Tejblum wrote: > Ruslan Ermilov wrote: > > Hi Dmitrij, > > On Sun, Sep 18, 2005 at 11:25:35PM +0400, Dmitrij Tejblum wrote: > > > When em_encap() tries to send a very long mbuf chain (i.e. more than > EM_MAX_SCATTER == 64 mbufs), bus_dmamap_load_mbuf_sg() may fail with EFBIG. > Then em_encap() fail, the packet is not sent and left in the output queue, > and thus no futher transmission is possible. > > Some other driver handle similar condition with m_defrag(9) function > (which is intended for this purpose). > > > > Can you please modify your patch as follows: > > 1) Count how much fragments are in the packet in em_encap() first, and > do m_defrag() if it exceeeds EM_MAX_SCATTER, like in if_dc.c. If it > is still EFBIG after that and bus_dmamap_load_mbuf_sg(), then free it > as you do to prevent re-enqueue. > > > So you think that if_dc.c is a better example than e.g. if_fxp.c and > if_xl.c? Why? I also suspect that your suggestion would be a > pessimisation. > Checking for a chain length shouldn't be much of pessimization, it's very fast, and I thought it'd save code duplication. But I don't insist on this one. If you would like to stay with try-and-fail approach, I don't mind. > 2) Put BPF processing back to em_start_locked(). > > > As I wrote, I think that if e.g. we were unable to defragment a packet it > is better to drop it and try to send the next, rather than stop and set > OACTIVE (since the code that clear OACTIVE assume that it is about TX > descriptors). (You didn't suggest to change that). I moved BPF processing > since it would not be a good idea to pass NULL to BPF. > I don't understand how moving BPF processing in em_encap() helps this, and how we can pass a NULL pointer if BPF processing stayed where it was, in start(). Also, in your patch, if either DMA loading of mbuf fails or mbuf chain defragmentation fails, NULL will be returned and no next packet will be processed (though I agree that in the latter case it would be logical to try the next packet). Cheers, -- Ruslan Ermilov ru@FreeBSD.org FreeBSD committer
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200509190810.j8J8AE5P096775>