* [PATCH 2/2] ixgbe: support skb->xmit_more in netdev_ops->ndo_start_xmit() @ 2014-08-25 23:35 David Miller 2014-08-26 15:00 ` Alexander Duyck 0 siblings, 1 reply; 6+ messages in thread From: David Miller @ 2014-08-25 23:35 UTC (permalink / raw) To: netdev Cc: therbert, jhs, hannes, edumazet, jeffrey.t.kirsher, rusty, dborkman, brouer From: Daniel Borkmann <dborkman@redhat.com> This implements the deferred tail pointer flush API for the ixgbe driver. Similar version also proposed longer time ago by Alexander Duyck. Signed-off-by: Daniel Borkmann <dborkman@redhat.com> Signed-off-by: David S. Miller <davem@davemloft.net> --- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c index 87bd53f..ba9ceaa 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c @@ -6958,9 +6958,10 @@ static void ixgbe_tx_map(struct ixgbe_ring *tx_ring, tx_ring->next_to_use = i; - /* notify HW of packet */ - ixgbe_write_tail(tx_ring, i); - + if (!skb->xmit_more) { + /* notify HW of packet */ + ixgbe_write_tail(tx_ring, i); + } return; dma_error: dev_err(tx_ring->dev, "TX DMA map failed\n"); -- 1.7.11.7 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ixgbe: support skb->xmit_more in netdev_ops->ndo_start_xmit() 2014-08-25 23:35 [PATCH 2/2] ixgbe: support skb->xmit_more in netdev_ops->ndo_start_xmit() David Miller @ 2014-08-26 15:00 ` Alexander Duyck 2014-08-26 15:40 ` Hannes Frederic Sowa 0 siblings, 1 reply; 6+ messages in thread From: Alexander Duyck @ 2014-08-26 15:00 UTC (permalink / raw) To: David Miller, netdev Cc: therbert, jhs, hannes, edumazet, jeffrey.t.kirsher, rusty, dborkman, brouer On 08/25/2014 04:35 PM, David Miller wrote: > > From: Daniel Borkmann <dborkman@redhat.com> > > This implements the deferred tail pointer flush API for the ixgbe > driver. Similar version also proposed longer time ago by Alexander Duyck. > > Signed-off-by: Daniel Borkmann <dborkman@redhat.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > --- > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > index 87bd53f..ba9ceaa 100644 > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > @@ -6958,9 +6958,10 @@ static void ixgbe_tx_map(struct ixgbe_ring *tx_ring, > > tx_ring->next_to_use = i; > > - /* notify HW of packet */ > - ixgbe_write_tail(tx_ring, i); > - > + if (!skb->xmit_more) { > + /* notify HW of packet */ > + ixgbe_write_tail(tx_ring, i); > + } > return; > dma_error: > dev_err(tx_ring->dev, "TX DMA map failed\n"); > It might help to add some handling for the case where xmit_more is set, but the ring has become full. This current implementation introduces the risk of triggering a Tx hang. My advice would be to pull the ixgbe_maybe_stop_tx code at the end of xmit_frame into the if check here, and perhaps look into adding an additional check to see if BQL has stopped the ring as well. Thanks, Alex ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ixgbe: support skb->xmit_more in netdev_ops->ndo_start_xmit() 2014-08-26 15:00 ` Alexander Duyck @ 2014-08-26 15:40 ` Hannes Frederic Sowa 2014-08-26 15:58 ` Daniel Borkmann 2014-08-26 16:20 ` Tom Herbert 0 siblings, 2 replies; 6+ messages in thread From: Hannes Frederic Sowa @ 2014-08-26 15:40 UTC (permalink / raw) To: Alexander Duyck Cc: David Miller, netdev, therbert, jhs, edumazet, jeffrey.t.kirsher, rusty, dborkman, brouer On Di, 2014-08-26 at 08:00 -0700, Alexander Duyck wrote: > On 08/25/2014 04:35 PM, David Miller wrote: > > > > From: Daniel Borkmann <dborkman@redhat.com> > > > > This implements the deferred tail pointer flush API for the ixgbe > > driver. Similar version also proposed longer time ago by Alexander Duyck. > > > > Signed-off-by: Daniel Borkmann <dborkman@redhat.com> > > Signed-off-by: David S. Miller <davem@davemloft.net> > > --- > > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > index 87bd53f..ba9ceaa 100644 > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > @@ -6958,9 +6958,10 @@ static void ixgbe_tx_map(struct ixgbe_ring *tx_ring, > > > > tx_ring->next_to_use = i; > > > > - /* notify HW of packet */ > > - ixgbe_write_tail(tx_ring, i); > > - > > + if (!skb->xmit_more) { > > + /* notify HW of packet */ > > + ixgbe_write_tail(tx_ring, i); > > + } > > return; > > dma_error: > > dev_err(tx_ring->dev, "TX DMA map failed\n"); > > > > It might help to add some handling for the case where xmit_more is set, > but the ring has become full. This current implementation introduces > the risk of triggering a Tx hang. IMHO this should be done before the patch lands in the driver. > My advice would be to pull the ixgbe_maybe_stop_tx code at the end of > xmit_frame into the if check here, and perhaps look into adding an > additional check to see if BQL has stopped the ring as well. I would like to have the BQL check not in the driver but in the generic code steering xmit_more. Otherwise I'll like the API. David, because of debugging and driver bugs, can we have an interface flag for that, so we can e.g. switch xmit_more to 0 permanently? It would also be nice to have for documentation purposes, so ethtool e.g. can discover feature-set of a driver. Thanks, Hannes ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ixgbe: support skb->xmit_more in netdev_ops->ndo_start_xmit() 2014-08-26 15:40 ` Hannes Frederic Sowa @ 2014-08-26 15:58 ` Daniel Borkmann 2014-08-26 16:20 ` Tom Herbert 1 sibling, 0 replies; 6+ messages in thread From: Daniel Borkmann @ 2014-08-26 15:58 UTC (permalink / raw) To: Hannes Frederic Sowa Cc: Alexander Duyck, David Miller, netdev, therbert, jhs, edumazet, jeffrey.t.kirsher, rusty, brouer On 08/26/2014 05:40 PM, Hannes Frederic Sowa wrote: > On Di, 2014-08-26 at 08:00 -0700, Alexander Duyck wrote: >> On 08/25/2014 04:35 PM, David Miller wrote: >>> >>> From: Daniel Borkmann <dborkman@redhat.com> >>> >>> This implements the deferred tail pointer flush API for the ixgbe >>> driver. Similar version also proposed longer time ago by Alexander Duyck. >>> >>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com> >>> Signed-off-by: David S. Miller <davem@davemloft.net> >>> --- >>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 7 ++++--- >>> 1 file changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >>> index 87bd53f..ba9ceaa 100644 >>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >>> @@ -6958,9 +6958,10 @@ static void ixgbe_tx_map(struct ixgbe_ring *tx_ring, >>> >>> tx_ring->next_to_use = i; >>> >>> - /* notify HW of packet */ >>> - ixgbe_write_tail(tx_ring, i); >>> - >>> + if (!skb->xmit_more) { >>> + /* notify HW of packet */ >>> + ixgbe_write_tail(tx_ring, i); >>> + } >>> return; >>> dma_error: >>> dev_err(tx_ring->dev, "TX DMA map failed\n"); >>> >> >> It might help to add some handling for the case where xmit_more is set, >> but the ring has become full. This current implementation introduces >> the risk of triggering a Tx hang. > > IMHO this should be done before the patch lands in the driver. I think that xmit_more=1 should be seen as an indication/hint to the driver so it can decide what to do with it, e.g. flush nevertheless when it runs under pressure. >> My advice would be to pull the ixgbe_maybe_stop_tx code at the end of >> xmit_frame into the if check here, and perhaps look into adding an >> additional check to see if BQL has stopped the ring as well. > > I would like to have the BQL check not in the driver but in the generic > code steering xmit_more. > > Otherwise I'll like the API. I agree with you that the logic when to set/unset the hint should come from a generic core part. I think the xmit_more API seems so far lightweight but lets see how it evolves. :) So far we don't have a flush-only part (as before), where one could trigger that outside of a fast-path to flush remaining skbs, but perhaps not having it is not too bad either. > David, because of debugging and driver bugs, can we have an interface > flag for that, so we can e.g. switch xmit_more to 0 permanently? It > would also be nice to have for documentation purposes, so ethtool e.g. > can discover feature-set of a driver. > > Thanks, > Hannes ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ixgbe: support skb->xmit_more in netdev_ops->ndo_start_xmit() 2014-08-26 15:40 ` Hannes Frederic Sowa 2014-08-26 15:58 ` Daniel Borkmann @ 2014-08-26 16:20 ` Tom Herbert 2014-08-26 16:36 ` Alexander Duyck 1 sibling, 1 reply; 6+ messages in thread From: Tom Herbert @ 2014-08-26 16:20 UTC (permalink / raw) To: Hannes Frederic Sowa Cc: Alexander Duyck, David Miller, Linux Netdev List, Jamal Hadi Salim, Eric Dumazet, Jeff Kirsher, Rusty Russell, Daniel Borkmann, brouer On Tue, Aug 26, 2014 at 8:40 AM, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > On Di, 2014-08-26 at 08:00 -0700, Alexander Duyck wrote: >> On 08/25/2014 04:35 PM, David Miller wrote: >> > >> > From: Daniel Borkmann <dborkman@redhat.com> >> > >> > This implements the deferred tail pointer flush API for the ixgbe >> > driver. Similar version also proposed longer time ago by Alexander Duyck. >> > >> > Signed-off-by: Daniel Borkmann <dborkman@redhat.com> >> > Signed-off-by: David S. Miller <davem@davemloft.net> >> > --- >> > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 7 ++++--- >> > 1 file changed, 4 insertions(+), 3 deletions(-) >> > >> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> > index 87bd53f..ba9ceaa 100644 >> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> > @@ -6958,9 +6958,10 @@ static void ixgbe_tx_map(struct ixgbe_ring *tx_ring, >> > >> > tx_ring->next_to_use = i; >> > >> > - /* notify HW of packet */ >> > - ixgbe_write_tail(tx_ring, i); >> > - >> > + if (!skb->xmit_more) { >> > + /* notify HW of packet */ >> > + ixgbe_write_tail(tx_ring, i); >> > + } >> > return; >> > dma_error: >> > dev_err(tx_ring->dev, "TX DMA map failed\n"); >> > >> >> It might help to add some handling for the case where xmit_more is set, >> but the ring has become full. This current implementation introduces >> the risk of triggering a Tx hang. > > IMHO this should be done before the patch lands in the driver. > >> My advice would be to pull the ixgbe_maybe_stop_tx code at the end of >> xmit_frame into the if check here, and perhaps look into adding an >> additional check to see if BQL has stopped the ring as well. > > I would like to have the BQL check not in the driver but in the generic > code steering xmit_more. > BQL stops the queue from netdev_tx_sent_queue, we could change that to return indication queue was stopped and use that as another check to flush. > Otherwise I'll like the API. > > David, because of debugging and driver bugs, can we have an interface > flag for that, so we can e.g. switch xmit_more to 0 permanently? It > would also be nice to have for documentation purposes, so ethtool e.g. > can discover feature-set of a driver. > > Thanks, > Hannes > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ixgbe: support skb->xmit_more in netdev_ops->ndo_start_xmit() 2014-08-26 16:20 ` Tom Herbert @ 2014-08-26 16:36 ` Alexander Duyck 0 siblings, 0 replies; 6+ messages in thread From: Alexander Duyck @ 2014-08-26 16:36 UTC (permalink / raw) To: Tom Herbert, Hannes Frederic Sowa Cc: David Miller, Linux Netdev List, Jamal Hadi Salim, Eric Dumazet, Jeff Kirsher, Rusty Russell, Daniel Borkmann, brouer On 08/26/2014 09:20 AM, Tom Herbert wrote: > On Tue, Aug 26, 2014 at 8:40 AM, Hannes Frederic Sowa > <hannes@stressinduktion.org> wrote: >> On Di, 2014-08-26 at 08:00 -0700, Alexander Duyck wrote: >>> On 08/25/2014 04:35 PM, David Miller wrote: >>>> >>>> From: Daniel Borkmann <dborkman@redhat.com> >>>> >>>> This implements the deferred tail pointer flush API for the ixgbe >>>> driver. Similar version also proposed longer time ago by Alexander Duyck. >>>> >>>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com> >>>> Signed-off-by: David S. Miller <davem@davemloft.net> >>>> --- >>>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 7 ++++--- >>>> 1 file changed, 4 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >>>> index 87bd53f..ba9ceaa 100644 >>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >>>> @@ -6958,9 +6958,10 @@ static void ixgbe_tx_map(struct ixgbe_ring *tx_ring, >>>> >>>> tx_ring->next_to_use = i; >>>> >>>> - /* notify HW of packet */ >>>> - ixgbe_write_tail(tx_ring, i); >>>> - >>>> + if (!skb->xmit_more) { >>>> + /* notify HW of packet */ >>>> + ixgbe_write_tail(tx_ring, i); >>>> + } >>>> return; >>>> dma_error: >>>> dev_err(tx_ring->dev, "TX DMA map failed\n"); >>>> >>> >>> It might help to add some handling for the case where xmit_more is set, >>> but the ring has become full. This current implementation introduces >>> the risk of triggering a Tx hang. >> >> IMHO this should be done before the patch lands in the driver. >> >>> My advice would be to pull the ixgbe_maybe_stop_tx code at the end of >>> xmit_frame into the if check here, and perhaps look into adding an >>> additional check to see if BQL has stopped the ring as well. >> >> I would like to have the BQL check not in the driver but in the generic >> code steering xmit_more. >> > > BQL stops the queue from netdev_tx_sent_queue, we could change that to > return indication queue was stopped and use that as another check to > flush. Either that or we could just add it as state check. Maybe change the code to something like: ixgbe_maybe_stop_tx(tx_ring, DESC_UNUSED); /* notify HW of packet */ if (netif_xmit_stopped(txring_txq(tx_ring)) || !skb->xmit_more) ixgbe_write_tail(tx_ring, i) Then we could catch both the driver or stack stopping the ring and identifying it as an indication that we should flush the ring. Thanks, Alex ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-08-26 16:36 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-08-25 23:35 [PATCH 2/2] ixgbe: support skb->xmit_more in netdev_ops->ndo_start_xmit() David Miller 2014-08-26 15:00 ` Alexander Duyck 2014-08-26 15:40 ` Hannes Frederic Sowa 2014-08-26 15:58 ` Daniel Borkmann 2014-08-26 16:20 ` Tom Herbert 2014-08-26 16:36 ` Alexander Duyck
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).