| Message ID | 1273400337-26501-6-git-send-email-vapier@gentoo.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Mike Frysinger <vapier@gentoo.org> Date: Sun, 9 May 2010 06:18:52 -0400 > From: Sonic Zhang <sonic.zhang@analog.com> > > Signed-off-by: Sonic Zhang <sonic.zhang@analog.com> > Signed-off-by: Mike Frysinger <vapier@gentoo.org> This should never ever happen, it's a bug and you should print a warning message when and if it does actually occur. At any point where your ->next pointer hits tx_list_head, the queue should have been stopped by your driver and therefore the networking core will never pass another packet to you. If this condition is actually triggering, it means you're not locking properly or you have some race. > --- > drivers/net/bfin_mac.c | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/bfin_mac.c b/drivers/net/bfin_mac.c > index 6d69bbb..0b5ea01 100644 > --- a/drivers/net/bfin_mac.c > +++ b/drivers/net/bfin_mac.c > @@ -920,6 +920,9 @@ static int bfin_mac_hard_start_xmit(struct sk_buff *skb, > u32 data_align = (unsigned long)(skb->data) & 0x3; > union skb_shared_tx *shtx = skb_tx(skb); > > + if (current_tx_ptr->next == tx_list_head) > + return NETDEV_TX_BUSY; > + > current_tx_ptr->skb = skb; > > if (data_align == 0x2) { > -- > 1.7.1 > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Sonic Zhang <sonic.adi@gmail.com> Date: Tue, 18 May 2010 18:52:17 +0800 [ Please never drop the mailing list when you're trying to discuss something networking related with me, thanks. I've put the CC: back. ] > On Mon, May 10, 2010 at 7:40 PM, David Miller <davem@davemloft.net> wrote: >> From: Mike Frysinger <vapier@gentoo.org> >> Date: Sun, 9 May 2010 06:18:52 -0400 >> >>> From: Sonic Zhang <sonic.zhang@analog.com> >>> >>> Signed-off-by: Sonic Zhang <sonic.zhang@analog.com> >>> Signed-off-by: Mike Frysinger <vapier@gentoo.org> >> >> This should never ever happen, it's a bug and you should print a warning >> message when and if it does actually occur. >> >> At any point where your ->next pointer hits tx_list_head, the queue >> should have been stopped by your driver and therefore the networking >> core will never pass another packet to you. > > To reduce the tx output overhead, the tx interrupt is not enabled and > handled in this driver. So, the driver doesn't know when to restart > the tx queue if the queue is stopped in ndo_start_xmit() at the point > where next pointer hits tx_list_head. > > In this case, although TX buffer list full rarely happens, the check > is still a safeguard. You can't do that, if you don't use the TX interrupt then you can leave SKBs stale in your TX ring for indefinite periods of time which is illegal. SKBs hold onto resources that can't be held indefinitely, such as TCP socket references and netfilter conntrack state. So if you leave a packet in your TX ring for a long time, there might be a TCP socket that now cannot be closed and freed up because of that. You must therefore free them very as soon as possible after the hardware is done with them. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 19, 2010 at 3:29 AM, David Miller <davem@davemloft.net> wrote: > From: Sonic Zhang <sonic.adi@gmail.com> > Date: Tue, 18 May 2010 18:52:17 +0800 > > [ Please never drop the mailing list when you're trying to > discuss something networking related with me, thanks. > I've put the CC: back. ] > >> On Mon, May 10, 2010 at 7:40 PM, David Miller <davem@davemloft.net> wrote: >>> From: Mike Frysinger <vapier@gentoo.org> >>> Date: Sun, 9 May 2010 06:18:52 -0400 >>> >>>> From: Sonic Zhang <sonic.zhang@analog.com> >>>> >>>> Signed-off-by: Sonic Zhang <sonic.zhang@analog.com> >>>> Signed-off-by: Mike Frysinger <vapier@gentoo.org> >>> >>> This should never ever happen, it's a bug and you should print a warning >>> message when and if it does actually occur. >>> >>> At any point where your ->next pointer hits tx_list_head, the queue >>> should have been stopped by your driver and therefore the networking >>> core will never pass another packet to you. >> >> To reduce the tx output overhead, the tx interrupt is not enabled and >> handled in this driver. So, the driver doesn't know when to restart >> the tx queue if the queue is stopped in ndo_start_xmit() at the point >> where next pointer hits tx_list_head. >> >> In this case, although TX buffer list full rarely happens, the check >> is still a safeguard. > > You can't do that, if you don't use the TX interrupt then you can leave > SKBs stale in your TX ring for indefinite periods of time which is > illegal. No, this doesn't happen, because before ndo_start_xmit() returns, the old TX buffers and skbs in the ring, which finished DMA operation, are freed. The only difference is that the free operation of a skb is done in next tx transfer. Sonic > > SKBs hold onto resources that can't be held indefinitely, such as TCP > socket references and netfilter conntrack state. So if you leave a > packet in your TX ring for a long time, there might be a TCP socket > that now cannot be closed and freed up because of that. > > You must therefore free them very as soon as possible after the > hardware is done with them. > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Sonic Zhang <sonic.adi@gmail.com> Date: Wed, 19 May 2010 17:23:16 +0800 > No, this doesn't happen, because before ndo_start_xmit() returns, the > old TX buffers and skbs in the ring, which finished DMA operation, are > freed. The only difference is that the free operation of a skb is done > in next tx transfer. This is still illegal. What if TX activity stops right then, and there is no "next tx transfer"? That SKB will never get freed, ever. You have to fix this. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 20, 2010 at 6:08 PM, David Miller <davem@davemloft.net> wrote: > From: Sonic Zhang <sonic.adi@gmail.com> > Date: Thu, 20 May 2010 17:38:07 +0800 > >> On Thu, May 20, 2010 at 4:12 AM, David Miller <davem@davemloft.net> wrote: >>> From: Sonic Zhang <sonic.adi@gmail.com> >>> Date: Wed, 19 May 2010 17:23:16 +0800 >>> >>>> No, this doesn't happen, because before ndo_start_xmit() returns, the >>>> old TX buffers and skbs in the ring, which finished DMA operation, are >>>> freed. The only difference is that the free operation of a skb is done >>>> in next tx transfer. >>> >>> This is still illegal. >>> >>> What if TX activity stops right then, and there is no "next tx >>> transfer"? >>> >> The skb remain in the TX ring will be freed finally when ndo_stop() is >> called to shutdown the network. So, this is not a problem. > > You really don't understand me, and I'm starting to get really > frustrated. You must free all packets in your TX ring in a very > small, finite, amount of time. This is not optional. And this > must happen regardless of what TX traffic which occurs in the future, > that means it must happen even if TX traffic suddenly stops. > > Your driver's behavior is absolutely not acceptable. > > Leaving the SKB In the TX ring like that means that potentially there > is a socket in the system or other major resource that cannot be released > and freed up. > > Please stop your driver from keeping packets in the TX ring indefinitely. > Forgot to CC netdev mailing list in my last reply. Try again. Sonic -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 20, 2010 at 6:08 PM, David Miller <davem@davemloft.net> wrote: > From: Sonic Zhang <sonic.adi@gmail.com> > Date: Thu, 20 May 2010 17:38:07 +0800 > >> On Thu, May 20, 2010 at 4:12 AM, David Miller <davem@davemloft.net> wrote: >>> From: Sonic Zhang <sonic.adi@gmail.com> >>> Date: Wed, 19 May 2010 17:23:16 +0800 >>> >>>> No, this doesn't happen, because before ndo_start_xmit() returns, the >>>> old TX buffers and skbs in the ring, which finished DMA operation, are >>>> freed. The only difference is that the free operation of a skb is done >>>> in next tx transfer. >>> >>> This is still illegal. >>> >>> What if TX activity stops right then, and there is no "next tx >>> transfer"? >>> >> The skb remain in the TX ring will be freed finally when ndo_stop() is >> called to shutdown the network. So, this is not a problem. > > You really don't understand me, and I'm starting to get really > frustrated. You must free all packets in your TX ring in a very > small, finite, amount of time. This is not optional. And this > must happen regardless of what TX traffic which occurs in the future, > that means it must happen even if TX traffic suddenly stops. > OK. I didn't figure out that the socket may not be closed if its skbs stay active somewhere in system for a long time. Thanks for your explanation. I will send a new patch to enabled tx interrupt and free skb from tx ring immediately. Sonic > Your driver's behavior is absolutely not acceptable. > > Leaving the SKB In the TX ring like that means that potentially there > is a socket in the system or other major resource that cannot be released > and freed up. > > Please stop your driver from keeping packets in the TX ring indefinitely. > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/bfin_mac.c b/drivers/net/bfin_mac.c index 6d69bbb..0b5ea01 100644 --- a/drivers/net/bfin_mac.c +++ b/drivers/net/bfin_mac.c @@ -920,6 +920,9 @@ static int bfin_mac_hard_start_xmit(struct sk_buff *skb, u32 data_align = (unsigned long)(skb->data) & 0x3; union skb_shared_tx *shtx = skb_tx(skb); + if (current_tx_ptr->next == tx_list_head) + return NETDEV_TX_BUSY; + current_tx_ptr->skb = skb; if (data_align == 0x2) {