Re: [PATCH v2 net-next 06/12] ep93xx_eth: add GRO support

[Eric Dumazet]

On Tue, May 16, 2017 at 1:41 PM, Alexander Sverdlin
 wrote:

> it turns out I've used this patch two weeks long already in 4.11; but I've 
> spent
> a couple of hours now torturing the new driver and was not able to provoke
> any inadequate behavior. It either receives all packets in time or not at all.
> If IRQs would be edge-triggered, I'd expect some stale packets, which do not
> arrive at first, but then appear with the packets coming next. This is not
> the case. I've used pktgen module for this, with minimal packets and
> different bursts.
>
> netperf shows 45Mbit/s on UDP_STREAM test, which is also fair amount for
> 200MHz CPU.
>
> So, I see no problems with the change.
>

Thanks a lot for testing !

Re: [PATCH v2 net-next 06/12] ep93xx_eth: add GRO support

[Alexander Sverdlin]

Hello all,

On 15/05/17 23:02, Alexander Sverdlin wrote:
>>> I don't know if we really care about this hardware anymore (I don't),
>>> but the ep93xx platform is still listed as being maintained in the
>>> MAINTAINERS file -- adding Ryan and Hartley.
>> I no longer have any ep93xx hardware to test with, and I never looked at
>> the Ethernet, so don't know the details. I think there are still a
>> handful of users. Adding Alexander who sent an ADC support series this
>> week, who might be able to test this?
> Yes, I very much care about ep93xx code being functional :)
> I'll test the patches tomorrow.

it turns out I've used this patch two weeks long already in 4.11; but I've spent
a couple of hours now torturing the new driver and was not able to provoke
any inadequate behavior. It either receives all packets in time or not at all.
If IRQs would be edge-triggered, I'd expect some stale packets, which do not
arrive at first, but then appear with the packets coming next. This is not
the case. I've used pktgen module for this, with minimal packets and
different bursts.

netperf shows 45Mbit/s on UDP_STREAM test, which is also fair amount for
200MHz CPU.

So, I see no problems with the change.

--
Alexander.

Re: [PATCH v2 net-next 06/12] ep93xx_eth: add GRO support

[Alexander Sverdlin]

Hi!

On Mon May 15 22:43:20 2017 Ryan Mallon  wrote:
> > I don't know if we really care about this hardware anymore (I don't),
> > but the ep93xx platform is still listed as being maintained in the
> > MAINTAINERS file -- adding Ryan and Hartley.
> 
> I no longer have any ep93xx hardware to test with, and I never looked at
> the Ethernet, so don't know the details. I think there are still a
> handful of users. Adding Alexander who sent an ADC support series this
> week, who might be able to test this?

Yes, I very much care about ep93xx code being functional :)
I'll test the patches tomorrow.

Alexander.

Re: [PATCH v2 net-next 06/12] ep93xx_eth: add GRO support

[Ryan Mallon]

On 15/05/17 20:31, Lennert Buytenhek wrote:
> On Sat, Feb 04, 2017 at 03:24:56PM -0800, Eric Dumazet wrote:
> 
>> Use napi_complete_done() instead of __napi_complete() to :
>>
>> 1) Get support of gro_flush_timeout if opt-in
>> 2) Not rearm interrupts for busy-polling users.
>> 3) use standard NAPI API.
>> 4) get rid of baroque code and ease maintenance.
>>
>> [...]
>>
>> @@ -310,35 +311,17 @@ static int ep93xx_rx(struct net_device *dev, int 
>> processed, int budget)
>>  return processed;
>>  }
>>  
>> -static int ep93xx_have_more_rx(struct ep93xx_priv *ep)
>> -{
>> -struct ep93xx_rstat *rstat = ep->descs->rstat + ep->rx_pointer;
>> -return !!((rstat->rstat0 & RSTAT0_RFP) && (rstat->rstat1 & RSTAT1_RFP));
>> -}
>> -
>>  static int ep93xx_poll(struct napi_struct *napi, int budget)
>>  {
>>  struct ep93xx_priv *ep = container_of(napi, struct ep93xx_priv, napi);
>>  struct net_device *dev = ep->dev;
>> -int rx = 0;
>> -
>> -poll_some_more:
>> -rx = ep93xx_rx(dev, rx, budget);
>> -if (rx < budget) {
>> -int more = 0;
>> +int rx;
>>  
>> +rx = ep93xx_rx(dev, budget);
>> +if (rx < budget && napi_complete_done(napi, rx)) {
>>  spin_lock_irq(>rx_lock);
>> -__napi_complete(napi);
>>  wrl(ep, REG_INTEN, REG_INTEN_TX | REG_INTEN_RX);
>> -if (ep93xx_have_more_rx(ep)) {
>> -wrl(ep, REG_INTEN, REG_INTEN_TX);
>> -wrl(ep, REG_INTSTSP, REG_INTSTS_RX);
>> -more = 1;
>> -}
>>  spin_unlock_irq(>rx_lock);
>> -
>> -if (more && napi_reschedule(napi))
>> -goto poll_some_more;
>>  }
>>  
>>  if (rx) {
> 
> This code was the way it was because the ep93xx hardware is somewhat
> braindead.  If I remember correctly (but it's been a while since I wrote
> this code):
> 
> 1. ep93xx netdev IRQs are edge-triggered, so if you re-enable IRQs
>while there was still work to be done, you will not get another IRQ.
> 
> 2. Disabling an interrupt source in the interrupt mask register will
>cause its interrupt status bit to always return zero, so you cannot
>check whether an interrupt status is pending without having the
>interrupt source enabled.
> 
> (I'll admit that a comment explaining this would have been in order.)
> 
> I don't know if we really care about this hardware anymore (I don't),
> but the ep93xx platform is still listed as being maintained in the
> MAINTAINERS file -- adding Ryan and Hartley.

I no longer have any ep93xx hardware to test with, and I never looked at
the Ethernet, so don't know the details. I think there are still a
handful of users. Adding Alexander who sent an ADC support series this
week, who might be able to test this?

~Ryan

Re: [PATCH v2 net-next 06/12] ep93xx_eth: add GRO support

[Lennert Buytenhek]

On Sat, Feb 04, 2017 at 03:24:56PM -0800, Eric Dumazet wrote:

> Use napi_complete_done() instead of __napi_complete() to :
> 
> 1) Get support of gro_flush_timeout if opt-in
> 2) Not rearm interrupts for busy-polling users.
> 3) use standard NAPI API.
> 4) get rid of baroque code and ease maintenance.
>
> [...]
>
> @@ -310,35 +311,17 @@ static int ep93xx_rx(struct net_device *dev, int 
> processed, int budget)
>   return processed;
>  }
>  
> -static int ep93xx_have_more_rx(struct ep93xx_priv *ep)
> -{
> - struct ep93xx_rstat *rstat = ep->descs->rstat + ep->rx_pointer;
> - return !!((rstat->rstat0 & RSTAT0_RFP) && (rstat->rstat1 & RSTAT1_RFP));
> -}
> -
>  static int ep93xx_poll(struct napi_struct *napi, int budget)
>  {
>   struct ep93xx_priv *ep = container_of(napi, struct ep93xx_priv, napi);
>   struct net_device *dev = ep->dev;
> - int rx = 0;
> -
> -poll_some_more:
> - rx = ep93xx_rx(dev, rx, budget);
> - if (rx < budget) {
> - int more = 0;
> + int rx;
>  
> + rx = ep93xx_rx(dev, budget);
> + if (rx < budget && napi_complete_done(napi, rx)) {
>   spin_lock_irq(>rx_lock);
> - __napi_complete(napi);
>   wrl(ep, REG_INTEN, REG_INTEN_TX | REG_INTEN_RX);
> - if (ep93xx_have_more_rx(ep)) {
> - wrl(ep, REG_INTEN, REG_INTEN_TX);
> - wrl(ep, REG_INTSTSP, REG_INTSTS_RX);
> - more = 1;
> - }
>   spin_unlock_irq(>rx_lock);
> -
> - if (more && napi_reschedule(napi))
> - goto poll_some_more;
>   }
>  
>   if (rx) {

This code was the way it was because the ep93xx hardware is somewhat
braindead.  If I remember correctly (but it's been a while since I wrote
this code):

1. ep93xx netdev IRQs are edge-triggered, so if you re-enable IRQs
   while there was still work to be done, you will not get another IRQ.

2. Disabling an interrupt source in the interrupt mask register will
   cause its interrupt status bit to always return zero, so you cannot
   check whether an interrupt status is pending without having the
   interrupt source enabled.

(I'll admit that a comment explaining this would have been in order.)

I don't know if we really care about this hardware anymore (I don't),
but the ep93xx platform is still listed as being maintained in the
MAINTAINERS file -- adding Ryan and Hartley.

[PATCH v2 net-next 06/12] ep93xx_eth: add GRO support

[Eric Dumazet]

Use napi_complete_done() instead of __napi_complete() to :

1) Get support of gro_flush_timeout if opt-in
2) Not rearm interrupts for busy-polling users.
3) use standard NAPI API.
4) get rid of baroque code and ease maintenance.

Signed-off-by: Eric Dumazet 
---
 drivers/net/ethernet/cirrus/ep93xx_eth.c | 29 ++---
 1 file changed, 6 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/cirrus/ep93xx_eth.c 
b/drivers/net/ethernet/cirrus/ep93xx_eth.c
index 
396c88678eabfec556536ca4a81f86224563..7a7c02f1f8b955f28206855f7084630cf8b3 
100644
--- a/drivers/net/ethernet/cirrus/ep93xx_eth.c
+++ b/drivers/net/ethernet/cirrus/ep93xx_eth.c
@@ -228,9 +228,10 @@ static void ep93xx_mdio_write(struct net_device *dev, int 
phy_id, int reg, int d
pr_info("mdio write timed out\n");
 }
 
-static int ep93xx_rx(struct net_device *dev, int processed, int budget)
+static int ep93xx_rx(struct net_device *dev, int budget)
 {
struct ep93xx_priv *ep = netdev_priv(dev);
+   int processed = 0;
 
while (processed < budget) {
int entry;
@@ -294,7 +295,7 @@ static int ep93xx_rx(struct net_device *dev, int processed, 
int budget)
skb_put(skb, length);
skb->protocol = eth_type_trans(skb, dev);
 
-   netif_receive_skb(skb);
+   napi_gro_receive(>napi, skb);
 
dev->stats.rx_packets++;
dev->stats.rx_bytes += length;
@@ -310,35 +311,17 @@ static int ep93xx_rx(struct net_device *dev, int 
processed, int budget)
return processed;
 }
 
-static int ep93xx_have_more_rx(struct ep93xx_priv *ep)
-{
-   struct ep93xx_rstat *rstat = ep->descs->rstat + ep->rx_pointer;
-   return !!((rstat->rstat0 & RSTAT0_RFP) && (rstat->rstat1 & RSTAT1_RFP));
-}
-
 static int ep93xx_poll(struct napi_struct *napi, int budget)
 {
struct ep93xx_priv *ep = container_of(napi, struct ep93xx_priv, napi);
struct net_device *dev = ep->dev;
-   int rx = 0;
-
-poll_some_more:
-   rx = ep93xx_rx(dev, rx, budget);
-   if (rx < budget) {
-   int more = 0;
+   int rx;
 
+   rx = ep93xx_rx(dev, budget);
+   if (rx < budget && napi_complete_done(napi, rx)) {
spin_lock_irq(>rx_lock);
-   __napi_complete(napi);
wrl(ep, REG_INTEN, REG_INTEN_TX | REG_INTEN_RX);
-   if (ep93xx_have_more_rx(ep)) {
-   wrl(ep, REG_INTEN, REG_INTEN_TX);
-   wrl(ep, REG_INTSTSP, REG_INTSTS_RX);
-   more = 1;
-   }
spin_unlock_irq(>rx_lock);
-
-   if (more && napi_reschedule(napi))
-   goto poll_some_more;
}
 
if (rx) {
-- 
2.11.0.483.g087da7b7c-goog