| Message ID | 1399243382-12528-6-git-send-email-soren.brinkmann@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
From: Soren Brinkmann <soren.brinkmann@xilinx.com> Date: Sun, 4 May 2014 15:43:02 -0700 > Under "heavy" RX load, the driver cannot handle the descriptors fast > enough. In detail, when a descriptor is consumed, its used flag is > cleared and once the RX budget is consumed all descriptors with a > cleared used flag are prepared to receive more data. Under load though, > the HW may constantly receive more data and use those descriptors with a > cleared used flag before they are actually prepared for next usage. > > The head and tail pointers into the RX-ring should always be valid and > we can omit clearing and checking of the used flag. > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> Isn't the RX_USED bit the only thing that controls what RX entries the chip will try to use? I can't see how you can just remove the RX_USED bit handling altogether. The problem actually seems to be that in the current code we clear the RX_USED bit before we actually reallocate the buffer and set it up. It should be a bug to see the RX_USED bit set in gem_rx_refill(), and the only reason why it can happen is exactly because you're clearing it too early in gem_rx(). This change doesn't seem to be correct, I'm not applying this series sorry.
On Mon, 2014-05-05 at 04:56PM -0400, David Miller wrote: > From: Soren Brinkmann <soren.brinkmann@xilinx.com> > Date: Sun, 4 May 2014 15:43:02 -0700 > > > Under "heavy" RX load, the driver cannot handle the descriptors fast > > enough. In detail, when a descriptor is consumed, its used flag is > > cleared and once the RX budget is consumed all descriptors with a > > cleared used flag are prepared to receive more data. Under load though, > > the HW may constantly receive more data and use those descriptors with a > > cleared used flag before they are actually prepared for next usage. > > > > The head and tail pointers into the RX-ring should always be valid and > > we can omit clearing and checking of the used flag. > > > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > > Isn't the RX_USED bit the only thing that controls what RX entries > the chip will try to use? > > I can't see how you can just remove the RX_USED bit handling > altogether. > > The problem actually seems to be that in the current code we clear the > RX_USED bit before we actually reallocate the buffer and set it up. > > It should be a bug to see the RX_USED bit set in gem_rx_refill(), and > the only reason why it can happen is exactly because you're clearing it > too early in gem_rx(). I don't follow. The HW uses the descriptor and the driver handles the received data. So, in gem_rx_refill we should actually only replace descriptor which have the RX_USED _set_, not? Currently the test tests for the opposite, since SW clears RX_USED in gem_rx. This patch just removes those two parts. The RX_USED is left as is (HW should have set it). And in gem_rx_refill we simply rely on the head and tail pointers to refill the used descriptors. I didn't see a reason to do the additional checking of the RX_USED bit. After the refill the RX_USED flags are of course cleared for all refilled descriptors. Thanks, Sören
From: Sören Brinkmann <soren.brinkmann@xilinx.com> Date: Mon, 5 May 2014 14:05:19 -0700 > On Mon, 2014-05-05 at 04:56PM -0400, David Miller wrote: >> From: Soren Brinkmann <soren.brinkmann@xilinx.com> >> Date: Sun, 4 May 2014 15:43:02 -0700 >> >> > Under "heavy" RX load, the driver cannot handle the descriptors fast >> > enough. In detail, when a descriptor is consumed, its used flag is >> > cleared and once the RX budget is consumed all descriptors with a >> > cleared used flag are prepared to receive more data. Under load though, >> > the HW may constantly receive more data and use those descriptors with a >> > cleared used flag before they are actually prepared for next usage. >> > >> > The head and tail pointers into the RX-ring should always be valid and >> > we can omit clearing and checking of the used flag. >> > >> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> >> >> Isn't the RX_USED bit the only thing that controls what RX entries >> the chip will try to use? >> >> I can't see how you can just remove the RX_USED bit handling >> altogether. >> >> The problem actually seems to be that in the current code we clear the >> RX_USED bit before we actually reallocate the buffer and set it up. >> >> It should be a bug to see the RX_USED bit set in gem_rx_refill(), and >> the only reason why it can happen is exactly because you're clearing it >> too early in gem_rx(). > > I don't follow. The HW uses the descriptor and the driver handles the > received data. So, in gem_rx_refill we should actually only replace > descriptor which have the RX_USED _set_, not? Currently the test tests > for the opposite, since SW clears RX_USED in gem_rx. This patch just > removes those two parts. The RX_USED is left as is (HW should have set > it). And in gem_rx_refill we simply rely on the head and tail pointers > to refill the used descriptors. I didn't see a reason to do the additional > checking of the RX_USED bit. > After the refill the RX_USED flags are of course cleared for all > refilled descriptors. Aha, that makes sense, I didn't notice that RX_USED is cleared as a side effect of gem_rx_refill()'s work. I'll apply this series, thanks for explaining.
On Mon, 2014-05-05 at 05:09PM -0400, David Miller wrote: > From: Sören Brinkmann <soren.brinkmann@xilinx.com> > Date: Mon, 5 May 2014 14:05:19 -0700 > > > On Mon, 2014-05-05 at 04:56PM -0400, David Miller wrote: > >> From: Soren Brinkmann <soren.brinkmann@xilinx.com> > >> Date: Sun, 4 May 2014 15:43:02 -0700 > >> > >> > Under "heavy" RX load, the driver cannot handle the descriptors fast > >> > enough. In detail, when a descriptor is consumed, its used flag is > >> > cleared and once the RX budget is consumed all descriptors with a > >> > cleared used flag are prepared to receive more data. Under load though, > >> > the HW may constantly receive more data and use those descriptors with a > >> > cleared used flag before they are actually prepared for next usage. > >> > > >> > The head and tail pointers into the RX-ring should always be valid and > >> > we can omit clearing and checking of the used flag. > >> > > >> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > >> > >> Isn't the RX_USED bit the only thing that controls what RX entries > >> the chip will try to use? > >> > >> I can't see how you can just remove the RX_USED bit handling > >> altogether. > >> > >> The problem actually seems to be that in the current code we clear the > >> RX_USED bit before we actually reallocate the buffer and set it up. > >> > >> It should be a bug to see the RX_USED bit set in gem_rx_refill(), and > >> the only reason why it can happen is exactly because you're clearing it > >> too early in gem_rx(). > > > > I don't follow. The HW uses the descriptor and the driver handles the > > received data. So, in gem_rx_refill we should actually only replace > > descriptor which have the RX_USED _set_, not? Currently the test tests > > for the opposite, since SW clears RX_USED in gem_rx. This patch just > > removes those two parts. The RX_USED is left as is (HW should have set > > it). And in gem_rx_refill we simply rely on the head and tail pointers > > to refill the used descriptors. I didn't see a reason to do the additional > > checking of the RX_USED bit. > > After the refill the RX_USED flags are of course cleared for all > > refilled descriptors. > > Aha, that makes sense, I didn't notice that RX_USED is cleared as a side > effect of gem_rx_refill()'s work. > > I'll apply this series, thanks for explaining. Thanks, but we probably wanna wait for Nicolas to test on his platforms using macb? Sören
From: Sören Brinkmann <soren.brinkmann@xilinx.com> Date: Mon, 5 May 2014 14:10:23 -0700 > On Mon, 2014-05-05 at 05:09PM -0400, David Miller wrote: >> From: Sören Brinkmann <soren.brinkmann@xilinx.com> >> Date: Mon, 5 May 2014 14:05:19 -0700 >> >> > On Mon, 2014-05-05 at 04:56PM -0400, David Miller wrote: >> >> From: Soren Brinkmann <soren.brinkmann@xilinx.com> >> >> Date: Sun, 4 May 2014 15:43:02 -0700 >> >> >> >> > Under "heavy" RX load, the driver cannot handle the descriptors fast >> >> > enough. In detail, when a descriptor is consumed, its used flag is >> >> > cleared and once the RX budget is consumed all descriptors with a >> >> > cleared used flag are prepared to receive more data. Under load though, >> >> > the HW may constantly receive more data and use those descriptors with a >> >> > cleared used flag before they are actually prepared for next usage. >> >> > >> >> > The head and tail pointers into the RX-ring should always be valid and >> >> > we can omit clearing and checking of the used flag. >> >> > >> >> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> >> >> >> >> Isn't the RX_USED bit the only thing that controls what RX entries >> >> the chip will try to use? >> >> >> >> I can't see how you can just remove the RX_USED bit handling >> >> altogether. >> >> >> >> The problem actually seems to be that in the current code we clear the >> >> RX_USED bit before we actually reallocate the buffer and set it up. >> >> >> >> It should be a bug to see the RX_USED bit set in gem_rx_refill(), and >> >> the only reason why it can happen is exactly because you're clearing it >> >> too early in gem_rx(). >> > >> > I don't follow. The HW uses the descriptor and the driver handles the >> > received data. So, in gem_rx_refill we should actually only replace >> > descriptor which have the RX_USED _set_, not? Currently the test tests >> > for the opposite, since SW clears RX_USED in gem_rx. This patch just >> > removes those two parts. The RX_USED is left as is (HW should have set >> > it). And in gem_rx_refill we simply rely on the head and tail pointers >> > to refill the used descriptors. I didn't see a reason to do the additional >> > checking of the RX_USED bit. >> > After the refill the RX_USED flags are of course cleared for all >> > refilled descriptors. >> >> Aha, that makes sense, I didn't notice that RX_USED is cleared as a side >> effect of gem_rx_refill()'s work. >> >> I'll apply this series, thanks for explaining. > > Thanks, but we probably wanna wait for Nicolas to test on his platforms > using macb? We can always apply a fixup or revert.
diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c index 3e13aa31548a..e9daa072ebb4 100644 --- a/drivers/net/ethernet/cadence/macb.c +++ b/drivers/net/ethernet/cadence/macb.c @@ -599,25 +599,16 @@ static void gem_rx_refill(struct macb *bp) { unsigned int entry; struct sk_buff *skb; - struct macb_dma_desc *desc; dma_addr_t paddr; while (CIRC_SPACE(bp->rx_prepared_head, bp->rx_tail, RX_RING_SIZE) > 0) { - u32 addr, ctrl; - entry = macb_rx_ring_wrap(bp->rx_prepared_head); - desc = &bp->rx_ring[entry]; /* Make hw descriptor updates visible to CPU */ rmb(); - addr = desc->addr; - ctrl = desc->ctrl; bp->rx_prepared_head++; - if ((addr & MACB_BIT(RX_USED))) - continue; - if (bp->rx_skbuff[entry] == NULL) { /* allocate sk_buff for this free entry in ring */ skb = netdev_alloc_skb(bp->dev, bp->rx_buffer_size); @@ -698,7 +689,6 @@ static int gem_rx(struct macb *bp, int budget) if (!(addr & MACB_BIT(RX_USED))) break; - desc->addr &= ~MACB_BIT(RX_USED); bp->rx_tail++; count++;
Under "heavy" RX load, the driver cannot handle the descriptors fast enough. In detail, when a descriptor is consumed, its used flag is cleared and once the RX budget is consumed all descriptors with a cleared used flag are prepared to receive more data. Under load though, the HW may constantly receive more data and use those descriptors with a cleared used flag before they are actually prepared for next usage. The head and tail pointers into the RX-ring should always be valid and we can omit clearing and checking of the used flag. Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> --- --- drivers/net/ethernet/cadence/macb.c | 10 ---------- 1 file changed, 10 deletions(-)