at91 driver lost CAN messages

at91 driver lost CAN messages

[Efim Monjak]

Hi all,

performance test at91_can.c driver by sending 4kByte data @1Mbit/s
without delay between CAN Frames show data lost by CAN driver. Sampling 
a Linux time stamps
show sometimes about 200usec delay between enter receive CAN interrupt 
and enter the
polling function by napi.
If napi polling is deactivated by follow change no more receive overflow 
error occurs.

Index: at91_can.c
===================================================================
--- at91_can.c    (revision 32447)
+++ at91_can.c    (revision 32448)
@@ -792,6 +792,35 @@
      return 1;
  }

+static int at91_poll_nonapi(struct net_device * dev, int quota)
+{
+    const struct at91_priv *priv = netdev_priv(dev);
+    u32 reg_sr = at91_read(priv, AT91_SR);
+    int work_done = 0;
+
+    if (reg_sr & get_irq_mb_rx(priv))
+        work_done += at91_poll_rx(dev, quota - work_done);
+
+    /*
+     * The error bits are clear on read,
+     * so use saved value from irq handler.
+     */
+    reg_sr |= priv->reg_sr;
+    if (reg_sr & AT91_IRQ_ERR_FRAME)
+        work_done += at91_poll_err(dev, quota - work_done, reg_sr);
+
+    if (work_done < quota) {
+        /* enable IRQs for frame errors and all mailboxes >= rx_next */
+        u32 reg_ier = AT91_IRQ_ERR_FRAME;
+        reg_ier |= get_irq_mb_rx(priv) & ~AT91_MB_MASK(priv->rx_next);
+
+        at91_write(priv, AT91_IER, reg_ier);
+    }
+
+    return work_done;
+}
+
+
  static int at91_poll(struct napi_struct *napi, int quota)
  {
      struct net_device *dev = napi->dev;
@@ -1086,7 +1115,15 @@
          priv->reg_sr = reg_sr;
          at91_write(priv, AT91_IDR,
                 get_irq_mb_rx(priv) | AT91_IRQ_ERR_FRAME);
-        napi_schedule(&priv->napi);
+
+        /*
+        * Call CAN controller polling function without napi.
+        * Sometimes need napi too many time to call
+        * the polling function. In this case Rx overflow
+        * occur at high bus load.
+        */
+        at91_poll_nonapi(dev, 64);
+        /* napi_schedule(&priv->napi); */
      }

      /* Transmission complete interrupt */


Is there a possibility to make using of napi configurable in this driver?

Depend on design sometimes after receive interrupt occurs
there is only time for three CAN frames before overflow error.
Not really a lot of time to use polling, but for other people possibly 
preferable.

Regards,
Efim

Re: at91 driver lost CAN messages

[Oliver Hartkopp]

Hi Efim,

On 03/10/2017 02:48 PM, Efim Monjak wrote:

> If napi polling is deactivated by follow change no more receive overflow
> error occurs.

thanks for the patch!

In fact I always thought NAPI is too heavy for CAN controllers. NAPI was 
intended for a very high interrupt load on some ethernet controllers 
with some considerable RX FIFO.

I have a 20 CAN interfaces (SJA1000 driver) Core i7 system receiving 
~22.000 frames/s without any problems, with an one-element FIFO and 
without NAPI.

@Marc: What was the reason to implement NAPI that days? Has it ever been 
proved that NAPI had a remarkable positive effect in opposite to a 
direct CAN frame processing in the interrupt context?

Regards,
Oliver

Re: at91 driver lost CAN messages

[Alexander Stein]

On Sunday 12 March 2017 00:06:02, Oliver Hartkopp wrote:
> Hi Efim,
> 
> On 03/10/2017 02:48 PM, Efim Monjak wrote:
> > If napi polling is deactivated by follow change no more receive overflow
> > error occurs.
> 
> thanks for the patch!
> 
> In fact I always thought NAPI is too heavy for CAN controllers. NAPI was
> intended for a very high interrupt load on some ethernet controllers
> with some considerable RX FIFO.
> 
> I have a 20 CAN interfaces (SJA1000 driver) Core i7 system receiving
> ~22.000 frames/s without any problems, with an one-element FIFO and
> without NAPI.
> 
> @Marc: What was the reason to implement NAPI that days? Has it ever been
> proved that NAPI had a remarkable positive effect in opposite to a
> direct CAN frame processing in the interrupt context?

One reason to use NAPI is out-of-order issues on SMP systems :-( You know the 
problem: http://marc.info/?l=linux-can&m=143642135416355&w=2
If this is not resolved, things get even worse.
Well, now that there is generic FIFO-offload feature (drivers may need changes 
to use it) there is an easy way to read CAN frames with low latency while 
processing them in non-IRQ context.
IMHO reading CAN frames in softirq (NAPI) may be too late in some cases, in RT 
kernels this may be even worse.

Best regards,
Alexander
-- 
Dipl.-Inf. Alexander Stein
SYS TEC electronic GmbH
alexander.stein@systec-electronic.com

Legal and Commercial Address:
Am Windrad 2
08468 Heinsdorfergrund
Germany

Office: +49 (0) 3765 38600-0
Fax:    +49 (0) 3765 38600-4100
 
Managing Directors:
	Director Technology/CEO: Dipl.-Phys. Siegmar Schmidt;
	Director Commercial Affairs/COO: Dipl. Ing. (FH) Armin von Collrepp
Commercial Registry:
	Amtsgericht Chemnitz, HRB 28082; USt.-Id Nr. DE150534010

Re: at91 driver lost CAN messages

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1045 bytes --]

On 03/12/2017 12:06 AM, Oliver Hartkopp wrote:
> thanks for the patch!
> 
> In fact I always thought NAPI is too heavy for CAN controllers. NAPI was 
> intended for a very high interrupt load on some ethernet controllers 
> with some considerable RX FIFO.
> 
> I have a 20 CAN interfaces (SJA1000 driver) Core i7 system receiving 
> ~22.000 frames/s without any problems, with an one-element FIFO and 
> without NAPI.
> 
> @Marc: What was the reason to implement NAPI that days? Has it ever been 
> proved that NAPI had a remarkable positive effect in opposite to a 
> direct CAN frame processing in the interrupt context?

On SMP systems the network stack doesn't seem to guarantee the order of
frames when put into the stack in IRQ context.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: at91 driver lost CAN messages

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1066 bytes --]

On 03/13/2017 07:54 AM, Alexander Stein wrote:
>> @Marc: What was the reason to implement NAPI that days? Has it ever been
>> proved that NAPI had a remarkable positive effect in opposite to a
>> direct CAN frame processing in the interrupt context?
> 
> One reason to use NAPI is out-of-order issues on SMP systems :-( You know the 
> problem: http://marc.info/?l=linux-can&m=143642135416355&w=2
> If this is not resolved, things get even worse.
> Well, now that there is generic FIFO-offload feature (drivers may need changes 
> to use it) there is an easy way to read CAN frames with low latency while 
> processing them in non-IRQ context.
> IMHO reading CAN frames in softirq (NAPI) may be too late in some cases, in RT 
> kernels this may be even worse.

ACK.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: at91 driver lost CAN messages

[Efim Monjak]

Am 13.03.2017 um 10:47 schrieb Marc Kleine-Budde:
> On 03/12/2017 12:06 AM, Oliver Hartkopp wrote:
>> thanks for the patch!
>>
>> In fact I always thought NAPI is too heavy for CAN controllers. NAPI was
>> intended for a very high interrupt load on some ethernet controllers
>> with some considerable RX FIFO.
>>
>> I have a 20 CAN interfaces (SJA1000 driver) Core i7 system receiving
>> ~22.000 frames/s without any problems, with an one-element FIFO and
>> without NAPI.
>>
>> @Marc: What was the reason to implement NAPI that days? Has it ever been
>> proved that NAPI had a remarkable positive effect in opposite to a
>> direct CAN frame processing in the interrupt context?
> On SMP systems the network stack doesn't seem to guarantee the order of
> frames when put into the stack in IRQ context.
>
> Marc
>

As I see Socketcan use a default queuing discipline pfifo_fast.
This use 3 bands with different priorities, may be here is a problem?

As I test my application I see a queue of about 163000 bytes can contain
only 320 CAN frames. It seems the fifo is not configure for CAN frames 
though
used PDU is 16 bytes.


-- 
Mit freundlichen Grüssen / Best regards
Efim Monjak

------------------------------------------------------------
Lipowsky Industrie-Elektronik GmbH
CAN- & LIN- TOOLS, Embedded Solutions
------------------------------------------------------------
Geschäftsführer/CEO: Andreas Lipowsky
Sitz/domicile:       64291 Darmstadt, Roemerstr. 57
Telefon/phone:       +49-(0)6151-93591-27
Telefax/fax:         +49-(0)6151-93591-28
Handelsregister      Darmstadt HRB 5139
USt-IDNr/VAT-ID:     DE 111647423
Quality Management:  DIN EN ISO 9001:2008
------------------------------------------------------------
Direktzugang: www.lipowsky.de

Re: at91 driver lost CAN messages

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1509 bytes --]

On 03/13/2017 01:55 PM, Efim Monjak wrote:
>>> In fact I always thought NAPI is too heavy for CAN controllers. NAPI was
>>> intended for a very high interrupt load on some ethernet controllers
>>> with some considerable RX FIFO.
>>>
>>> I have a 20 CAN interfaces (SJA1000 driver) Core i7 system receiving
>>> ~22.000 frames/s without any problems, with an one-element FIFO and
>>> without NAPI.
>>>
>>> @Marc: What was the reason to implement NAPI that days? Has it ever been
>>> proved that NAPI had a remarkable positive effect in opposite to a
>>> direct CAN frame processing in the interrupt context?
>> On SMP systems the network stack doesn't seem to guarantee the order of
>> frames when put into the stack in IRQ context.

> As I see Socketcan use a default queuing discipline pfifo_fast.
> This use 3 bands with different priorities, may be here is a problem?

No, queue disciplines are in the TX path, I was talking about RX.

> As I test my application I see a queue of about 163000 bytes can
> contain only 320 CAN frames. It seems the fifo is not configure for
> CAN frames though used PDU is 16 bytes.

Which queue do you mean exactly? Maybe it's the sum of skbs not the
can_frames.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: at91 driver lost CAN messages

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1660 bytes --]

On 03/13/2017 04:17 PM, Efim Monjak wrote:
> I am using Kernel 4.4.27. By getsocketopt() it is possible to get 
> maximum receive buffer size in bytes.

Yes, that limits the memory that's used for skbs, not only the CAN frames.

> if (getsockopt(sock, SOL_SOCKET, SO_RCVBUF, (void*)&n, &m) == 0)      
> fprintf(stderr, "\nCAN Rx Bufsize %u bytes", n);
> 
> As I understand as long as no CAN frame is read from socket by user 
> application all received CAN frames will be stored into this buffer by 
> CAN driver.

Not directly by the CAN driver, but by the networking stack.

> If more CAN frames are received as receive buffer able to
> contain a buffer overflow occurs and CAN frames goes lost.

Yes, on this socket the CAN frames are lost.

> For test proposes the application opened  a socket and was stopped by debugger
> after it. The result of getsockopt() was a bit more as 163000 bytes. 
> After it CAN frames was sent for about 1..2 min. It is enough time to 
> get receive buffer overflow. Now sending of CAN frames was stopped and 
> the application have read all CAN frames stored in the socket and 
> counted them. It was 320. For test I have set size of receive buffer to 
> value was read from socket (about 163000). After it number of received 
> CAN frames was 640.
> I think skbs are stored into receive buffer/queue.

yes

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: at91 driver lost CAN messages

[Efim Monjak]

On 03/13/2017 02:11 PM, Marc Kleine-Budde wrote:
> On 03/13/2017 01:55 PM, Efim Monjak wrote:
>>>> In fact I always thought NAPI is too heavy for CAN controllers. NAPI was
>>>> intended for a very high interrupt load on some ethernet controllers
>>>> with some considerable RX FIFO.
>>>>
>>>> I have a 20 CAN interfaces (SJA1000 driver) Core i7 system receiving
>>>> ~22.000 frames/s without any problems, with an one-element FIFO and
>>>> without NAPI.
>>>>
>>>> @Marc: What was the reason to implement NAPI that days? Has it ever been
>>>> proved that NAPI had a remarkable positive effect in opposite to a
>>>> direct CAN frame processing in the interrupt context?
>>> On SMP systems the network stack doesn't seem to guarantee the order of
>>> frames when put into the stack in IRQ context.
>> As I see Socketcan use a default queuing discipline pfifo_fast.
>> This use 3 bands with different priorities, may be here is a problem?
> No, queue disciplines are in the TX path, I was talking about RX.
>
>> As I test my application I see a queue of about 163000 bytes can
>> contain only 320 CAN frames. It seems the fifo is not configure for
>> CAN frames though used PDU is 16 bytes.
> Which queue do you mean exactly? Maybe it's the sum of skbs not the
> can_frames.
>
> Marc
>
I am using Kernel 4.4.27. By getsocketopt() it is possible to get 
maximum receive buffer size in bytes.

if (getsockopt(sock, SOL_SOCKET, SO_RCVBUF, (void*)&n, &m) == 0)      
fprintf(stderr, "\nCAN Rx Bufsize %u bytes", n);

As I understand as long as no CAN frame is read from socket by user 
application all received CAN frames will be stored into this buffer by 
CAN driver. If more CAN frames are received as receive buffer able to 
contain a buffer overflow occurs and CAN frames goes lost.
For test proposes the application opened  a socket and was stopped by debugger
after it. The result of getsockopt() was a bit more as 163000 bytes. 
After it CAN frames was sent for about 1..2 min. It is enough time to 
get receive buffer overflow. Now sending of CAN frames was stopped and 
the application have read all CAN frames stored in the socket and 
counted them. It was 320. For test I have set size of receive buffer to 
value was read from socket (about 163000). After it number of received 
CAN frames was 640.
I think skbs are stored into receive buffer/queue.

Efim

Re: at91 driver lost CAN messages

[Oliver Hartkopp]

On 13.03.2017 10:47, Marc Kleine-Budde wrote:
> On 03/13/2017 07:54 AM, Alexander Stein wrote:
>>> @Marc: What was the reason to implement NAPI that days? Has it ever been
>>> proved that NAPI had a remarkable positive effect in opposite to a
>>> direct CAN frame processing in the interrupt context?
>>
>> One reason to use NAPI is out-of-order issues on SMP systems :-( You know the
>> problem: http://marc.info/?l=linux-can&m=143642135416355&w=2
>> If this is not resolved, things get even worse.
>> Well, now that there is generic FIFO-offload feature (drivers may need changes
>> to use it) there is an easy way to read CAN frames with low latency while
>> processing them in non-IRQ context.
>> IMHO reading CAN frames in softirq (NAPI) may be too late in some cases, in RT
>> kernels this may be even worse.
>
> ACK.

Would then implementing CAN rx-offload on all(!) CAN drivers fix the 
NAPI-too-late and the out-of-order issue?

Regards,
Oliver

Re: at91 driver lost CAN messages

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1642 bytes --]

On 03/16/2017 09:01 AM, Oliver Hartkopp wrote:
> On 13.03.2017 10:47, Marc Kleine-Budde wrote:
>> On 03/13/2017 07:54 AM, Alexander Stein wrote:
>>>> @Marc: What was the reason to implement NAPI that days? Has it ever been
>>>> proved that NAPI had a remarkable positive effect in opposite to a
>>>> direct CAN frame processing in the interrupt context?
>>>
>>> One reason to use NAPI is out-of-order issues on SMP systems :-( You know the
>>> problem: http://marc.info/?l=linux-can&m=143642135416355&w=2
>>> If this is not resolved, things get even worse.
>>> Well, now that there is generic FIFO-offload feature (drivers may need changes
>>> to use it) there is an easy way to read CAN frames with low latency while
>>> processing them in non-IRQ context.
>>> IMHO reading CAN frames in softirq (NAPI) may be too late in some cases, in RT
>>> kernels this may be even worse.
>>
>> ACK.
> 
> Would then implementing CAN rx-offload on all(!) CAN drivers fix the 
> NAPI-too-late and the out-of-order issue?

Implementing rx-offload on the drivers using NAPI, will fix the
NAPI-too-late issue (and turn it into a IRQ too late issue).

Implementing rx-offload on the drivers passing skbs in the IRQ handler
will probably fix the out-of-order issue.

I'm not sure if the USB and SPI drivers are affected by the out-of-order
issue.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: at91 driver lost CAN messages

[Alexander Stein]

On Thursday 16 March 2017 09:34:38, Marc Kleine-Budde wrote:
> On 03/16/2017 09:01 AM, Oliver Hartkopp wrote:
> > On 13.03.2017 10:47, Marc Kleine-Budde wrote:
> >> On 03/13/2017 07:54 AM, Alexander Stein wrote:
> >>>> @Marc: What was the reason to implement NAPI that days? Has it ever
> >>>> been
> >>>> proved that NAPI had a remarkable positive effect in opposite to a
> >>>> direct CAN frame processing in the interrupt context?
> >>> 
> >>> One reason to use NAPI is out-of-order issues on SMP systems :-( You
> >>> know the problem: http://marc.info/?l=linux-can&m=143642135416355&w=2
> >>> If this is not resolved, things get even worse.
> >>> Well, now that there is generic FIFO-offload feature (drivers may need
> >>> changes to use it) there is an easy way to read CAN frames with low
> >>> latency while processing them in non-IRQ context.
> >>> IMHO reading CAN frames in softirq (NAPI) may be too late in some cases,
> >>> in RT kernels this may be even worse.
> >> 
> >> ACK.
> > 
> > Would then implementing CAN rx-offload on all(!) CAN drivers fix the
> > NAPI-too-late and the out-of-order issue?
> 
> Implementing rx-offload on the drivers using NAPI, will fix the
> NAPI-too-late issue (and turn it into a IRQ too late issue).
> 
> Implementing rx-offload on the drivers passing skbs in the IRQ handler
> will probably fix the out-of-order issue.
> 
> I'm not sure if the USB and SPI drivers are affected by the out-of-order
> issue.

IMO all drivers using netif_rx (IRQ context) rather han netif_receive_skb 
(NAPI poll context) are suffering from out-of-order issue. And I guess every 
USB CAN driver is using netif_rx in the URB callback function, which is 
essentially IRQ context. MCP2512 uses netif_rx_ni instead, dunno what is the 
actual difference to netif_rx.

Let's see if I get time to cook rx-offload on a USB driver.

Best regards,
Alexander
-- 
Dipl.-Inf. Alexander Stein
SYS TEC electronic GmbH
alexander.stein@systec-electronic.com

Legal and Commercial Address:
Am Windrad 2
08468 Heinsdorfergrund
Germany

Office: +49 (0) 3765 38600-0
Fax:    +49 (0) 3765 38600-4100
 
Managing Directors:
	Director Technology/CEO: Dipl.-Phys. Siegmar Schmidt;
	Director Commercial Affairs/COO: Dipl. Ing. (FH) Armin von Collrepp
Commercial Registry:
	Amtsgericht Chemnitz, HRB 28082; USt.-Id Nr. DE150534010

Re: at91 driver lost CAN messages

[Oliver Hartkopp]

On 16.03.2017 09:34, Marc Kleine-Budde wrote:
> On 03/16/2017 09:01 AM, Oliver Hartkopp wrote:
>> On 13.03.2017 10:47, Marc Kleine-Budde wrote:
>>> On 03/13/2017 07:54 AM, Alexander Stein wrote:
>>>>> @Marc: What was the reason to implement NAPI that days? Has it ever been
>>>>> proved that NAPI had a remarkable positive effect in opposite to a
>>>>> direct CAN frame processing in the interrupt context?
>>>>
>>>> One reason to use NAPI is out-of-order issues on SMP systems :-( You know the
>>>> problem: http://marc.info/?l=linux-can&m=143642135416355&w=2
>>>> If this is not resolved, things get even worse.
>>>> Well, now that there is generic FIFO-offload feature (drivers may need changes
>>>> to use it) there is an easy way to read CAN frames with low latency while
>>>> processing them in non-IRQ context.
>>>> IMHO reading CAN frames in softirq (NAPI) may be too late in some cases, in RT
>>>> kernels this may be even worse.
>>>
>>> ACK.
>>
>> Would then implementing CAN rx-offload on all(!) CAN drivers fix the
>> NAPI-too-late and the out-of-order issue?
>
> Implementing rx-offload on the drivers using NAPI, will fix the
> NAPI-too-late issue (and turn it into a IRQ too late issue).
>
> Implementing rx-offload on the drivers passing skbs in the IRQ handler
> will probably fix the out-of-order issue.
>
> I'm not sure if the USB and SPI drivers are affected by the out-of-order
> issue.

The are :-(

As they don't use NAPI the incomming skb can processed by any CPU.

Regards,
Oliver

Re: at91 driver lost CAN messages

[Oliver Hartkopp]

On 16.03.2017 10:06, Alexander Stein wrote:
> On Thursday 16 March 2017 09:34:38, Marc Kleine-Budde wrote:
>> On 03/16/2017 09:01 AM, Oliver Hartkopp wrote:
>>> On 13.03.2017 10:47, Marc Kleine-Budde wrote:
>>>> On 03/13/2017 07:54 AM, Alexander Stein wrote:
>>>>>> @Marc: What was the reason to implement NAPI that days? Has it ever
>>>>>> been
>>>>>> proved that NAPI had a remarkable positive effect in opposite to a
>>>>>> direct CAN frame processing in the interrupt context?
>>>>>
>>>>> One reason to use NAPI is out-of-order issues on SMP systems :-( You
>>>>> know the problem: http://marc.info/?l=linux-can&m=143642135416355&w=2
>>>>> If this is not resolved, things get even worse.
>>>>> Well, now that there is generic FIFO-offload feature (drivers may need
>>>>> changes to use it) there is an easy way to read CAN frames with low
>>>>> latency while processing them in non-IRQ context.
>>>>> IMHO reading CAN frames in softirq (NAPI) may be too late in some cases,
>>>>> in RT kernels this may be even worse.
>>>>
>>>> ACK.
>>>
>>> Would then implementing CAN rx-offload on all(!) CAN drivers fix the
>>> NAPI-too-late and the out-of-order issue?
>>
>> Implementing rx-offload on the drivers using NAPI, will fix the
>> NAPI-too-late issue (and turn it into a IRQ too late issue).
>>
>> Implementing rx-offload on the drivers passing skbs in the IRQ handler
>> will probably fix the out-of-order issue.
>>
>> I'm not sure if the USB and SPI drivers are affected by the out-of-order
>> issue.
>
> IMO all drivers using netif_rx (IRQ context) rather han netif_receive_skb
> (NAPI poll context) are suffering from out-of-order issue. And I guess every
> USB CAN driver is using netif_rx in the URB callback function, which is
> essentially IRQ context. MCP2512 uses netif_rx_ni instead, dunno what is the
> actual difference to netif_rx.
>
> Let's see if I get time to cook rx-offload on a USB driver.

That would be highly appreciated :-)

Best regards,
Oliver

Re: at91 driver lost CAN messages

[Efim Monjak]

Am 16.03.2017 um 09:34 schrieb Marc Kleine-Budde:
> On 03/16/2017 09:01 AM, Oliver Hartkopp wrote:
>> On 13.03.2017 10:47, Marc Kleine-Budde wrote:
>>> On 03/13/2017 07:54 AM, Alexander Stein wrote:
>>>>> @Marc: What was the reason to implement NAPI that days? Has it ever been
>>>>> proved that NAPI had a remarkable positive effect in opposite to a
>>>>> direct CAN frame processing in the interrupt context?
>>>> One reason to use NAPI is out-of-order issues on SMP systems :-( You know the
>>>> problem: http://marc.info/?l=linux-can&m=143642135416355&w=2
>>>> If this is not resolved, things get even worse.
>>>> Well, now that there is generic FIFO-offload feature (drivers may need changes
>>>> to use it) there is an easy way to read CAN frames with low latency while
>>>> processing them in non-IRQ context.
>>>> IMHO reading CAN frames in softirq (NAPI) may be too late in some cases, in RT
>>>> kernels this may be even worse.
>>> ACK.
>> Would then implementing CAN rx-offload on all(!) CAN drivers fix the
>> NAPI-too-late and the out-of-order issue?
> Implementing rx-offload on the drivers using NAPI, will fix the
> NAPI-too-late issue (and turn it into a IRQ too late issue).

It seems the napi in AT91 driver is called from CAN ISR. In this case 
here are
both issues.

> Implementing rx-offload on the drivers passing skbs in the IRQ handler
> will probably fix the out-of-order issue.

Were possible by rx-offload implementing make skbs no such memory 
wasting for CAN
data?


> I'm not sure if the USB and SPI drivers are affected by the out-of-order
> issue.
>
> Marc
>

Regards,
Efim

Re: at91 driver lost CAN messages

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1201 bytes --]

On 03/16/2017 10:24 AM, Efim Monjak wrote:
>>> Would then implementing CAN rx-offload on all(!) CAN drivers fix the
>>> NAPI-too-late and the out-of-order issue?
>> Implementing rx-offload on the drivers using NAPI, will fix the
>> NAPI-too-late issue (and turn it into a IRQ too late issue).
> 
> It seems the napi in AT91 driver is called from CAN ISR. In this case
> here are both issues.

NAPI is supposed to be activated once you get an interrupt. But the
actual read of the messages and putting it into the networking stack is
done in NAPI, which comes after the IRQ.

>> Implementing rx-offload on the drivers passing skbs in the IRQ handler
>> will probably fix the out-of-order issue.
> 
> Were possible by rx-offload implementing make skbs no such memory 
> wasting for CAN data?

I'm not sure if I understand your question correctly. In CAN you need
still one skb for each CAN frame.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: at91 driver lost CAN messages

[Alexander Stein]

On Thursday 16 March 2017 10:09:30, Oliver Hartkopp wrote:
> On 16.03.2017 10:06, Alexander Stein wrote:
> > On Thursday 16 March 2017 09:34:38, Marc Kleine-Budde wrote:
> >> On 03/16/2017 09:01 AM, Oliver Hartkopp wrote:
> >>> On 13.03.2017 10:47, Marc Kleine-Budde wrote:
> >>>> On 03/13/2017 07:54 AM, Alexander Stein wrote:
> >>>>>> @Marc: What was the reason to implement NAPI that days? Has it ever
> >>>>>> been
> >>>>>> proved that NAPI had a remarkable positive effect in opposite to a
> >>>>>> direct CAN frame processing in the interrupt context?
> >>>>> 
> >>>>> One reason to use NAPI is out-of-order issues on SMP systems :-( You
> >>>>> know the problem: http://marc.info/?l=linux-can&m=143642135416355&w=2
> >>>>> If this is not resolved, things get even worse.
> >>>>> Well, now that there is generic FIFO-offload feature (drivers may need
> >>>>> changes to use it) there is an easy way to read CAN frames with low
> >>>>> latency while processing them in non-IRQ context.
> >>>>> IMHO reading CAN frames in softirq (NAPI) may be too late in some
> >>>>> cases,
> >>>>> in RT kernels this may be even worse.
> >>>> 
> >>>> ACK.
> >>> 
> >>> Would then implementing CAN rx-offload on all(!) CAN drivers fix the
> >>> NAPI-too-late and the out-of-order issue?
> >> 
> >> Implementing rx-offload on the drivers using NAPI, will fix the
> >> NAPI-too-late issue (and turn it into a IRQ too late issue).
> >> 
> >> Implementing rx-offload on the drivers passing skbs in the IRQ handler
> >> will probably fix the out-of-order issue.
> >> 
> >> I'm not sure if the USB and SPI drivers are affected by the out-of-order
> >> issue.
> > 
> > IMO all drivers using netif_rx (IRQ context) rather han netif_receive_skb
> > (NAPI poll context) are suffering from out-of-order issue. And I guess
> > every USB CAN driver is using netif_rx in the URB callback function,
> > which is essentially IRQ context. MCP2512 uses netif_rx_ni instead, dunno
> > what is the actual difference to netif_rx.
> > 
> > Let's see if I get time to cook rx-offload on a USB driver.
> 
> That would be highly appreciated :-)

Mh, at a first glance I don't know how to do that, actually. AFAICS rx-offload 
does not support 'pushing' struct can_frame into the queue, only pulling from 
rx-offload. There is no fixed connection bewteen the internal can device 
structure and CAN frames, like hardware message boxes.
In order to use can_rx_offload_irq_offload_fifo() to insert (mostly) a single 
can_frame I would need to store the last urb. I think I need to add a function 
similar to can_rx_offload_irq_queue_err_skb(), e.g. 
can_rx_offload_irq_queue_skb, but without scheduling. This should be done once 
the urb was completly read.

Marc: Does that sound reasonable?

Best regards,
Alexander
-- 
Dipl.-Inf. Alexander Stein
SYS TEC electronic GmbH
alexander.stein@systec-electronic.com

Legal and Commercial Address:
Am Windrad 2
08468 Heinsdorfergrund
Germany

Office: +49 (0) 3765 38600-0
Fax:    +49 (0) 3765 38600-4100
 
Managing Directors:
	Director Technology/CEO: Dipl.-Phys. Siegmar Schmidt;
	Director Commercial Affairs/COO: Dipl. Ing. (FH) Armin von Collrepp
Commercial Registry:
	Amtsgericht Chemnitz, HRB 28082; USt.-Id Nr. DE150534010

Re: at91 driver lost CAN messages

[Efim Monjak]

Am 16.03.2017 um 10:32 schrieb Marc Kleine-Budde:
> On 03/16/2017 10:24 AM, Efim Monjak wrote:
>>>> Would then implementing CAN rx-offload on all(!) CAN drivers fix the
>>>> NAPI-too-late and the out-of-order issue?
>>> Implementing rx-offload on the drivers using NAPI, will fix the
>>> NAPI-too-late issue (and turn it into a IRQ too late issue).
>> It seems the napi in AT91 driver is called from CAN ISR. In this case
>> here are both issues.
> NAPI is supposed to be activated once you get an interrupt. But the
> actual read of the messages and putting it into the networking stack is
> done in NAPI, which comes after the IRQ.

Yes using of napi contains both issues NAPI-too-late and IRQ-too-late.

>
>>> Implementing rx-offload on the drivers passing skbs in the IRQ handler
>>> will probably fix the out-of-order issue.
>> Were possible by rx-offload implementing make skbs no such memory
>> wasting for CAN data?
> I'm not sure if I understand your question correctly. In CAN you need
> still one skb for each CAN frame.
>
> Marc
>

Yes an skb contains one CAN frame. As I have written early socket show 
me receive buffer
is about 163000 bytes. After overflow 320 CAN frames are read from 
socket. I think
receive buffer is full in this case. If no place in receive buffer is 
used for other proposes
163000bytes/320 skbs with CAN frames make about 500 bytes/skb.
Linux show CAN MTU is 16 byte big.
What I mean: need an skb about a half of kbyte to contain 16 bytes?
Sure skbs are parts of list but do they need this all memory?


Regards,
Efim

Re: at91 driver lost CAN messages

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1769 bytes --]

On 03/16/2017 11:06 AM, Efim Monjak wrote:
>> NAPI is supposed to be activated once you get an interrupt. But the
>> actual read of the messages and putting it into the networking stack is
>> done in NAPI, which comes after the IRQ.
> 
> Yes using of napi contains both issues NAPI-too-late and IRQ-too-late.

Yes - but IRQ-too-late cannot be fixed or worked around in the driver.

>>>> Implementing rx-offload on the drivers passing skbs in the IRQ handler
>>>> will probably fix the out-of-order issue.
>>> Were possible by rx-offload implementing make skbs no such memory
>>> wasting for CAN data?
>> I'm not sure if I understand your question correctly. In CAN you need
>> still one skb for each CAN frame.

> Yes an skb contains one CAN frame. As I have written early socket
> show me receive buffer is about 163000 bytes. After overflow 320 CAN
> frames are read from socket. I think receive buffer is full in this
> case. If no place in receive buffer is used for other proposes 
> 163000bytes/320 skbs with CAN frames make about 500 bytes/skb. Linux
> show CAN MTU is 16 byte big. What I mean: need an skb about a half of
> kbyte to contain 16 bytes? Sure skbs are parts of list but do they
> need this all memory?

Yes - that's the overhead we accept for making use of the socket
infrastructure in linux. Feel free to dig into the possibilities of
aggregating several CAN frames into a single skb. On the ethernet side
they have things like GRO.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: at91 driver lost CAN messages

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1955 bytes --]

On 03/16/2017 10:58 AM, Alexander Stein wrote:
>>> Let's see if I get time to cook rx-offload on a USB driver.
>> 
>> That would be highly appreciated :-)
> 
> Mh, at a first glance I don't know how to do that, actually. AFAICS
> rx-offload does not support 'pushing' struct can_frame into the
> queue, only pulling from rx-offload. There is no fixed connection
> bewteen the internal can device structure and CAN frames, like
> hardware message boxes. In order to use
> can_rx_offload_irq_offload_fifo() to insert (mostly) a single 
> can_frame I would need to store the last urb. I think I need to add a
> function similar to can_rx_offload_irq_queue_err_skb(), e.g. 
> can_rx_offload_irq_queue_skb, but without scheduling. This should be
> done once the urb was completly read.

Does your device support multiple CAN frames per USB?

Maybe rename

> int can_rx_offload_irq_queue_err_skb(struct can_rx_offload *offload, struct sk_buff *skb)

into can_rx_offload_irq_queue_skb() or
can_rx_offload_irq_queue_skb_schedule() and fix the race condition:

> int can_rx_offload_irq_queue_skb(struct can_rx_offload *offload, struct sk_buff *skb)
> {
take spinlock
>         if (skb_queue_len(&offload->skb_queue) >
>             offload->skb_queue_len_max)

release in case of error

>                 return -ENOMEM;
> 
>         skb_queue_tail(&offload->skb_queue, skb);

use unlocked variant of skb_queue_tail, i.e. __skb_queue_tail()

release spin lock

>         can_rx_offload_schedule(offload);
> 
>         return 0;
> }

Then factor all but the can_rx_offload_schedule(offload) call into a
separate function.

regards,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: at91 driver lost CAN messages

[Alexander Stein]

On Thursday 16 March 2017 11:18:33, Marc Kleine-Budde wrote:
> On 03/16/2017 10:58 AM, Alexander Stein wrote:
> >>> Let's see if I get time to cook rx-offload on a USB driver.
> >> 
> >> That would be highly appreciated :-)
> > 
> > Mh, at a first glance I don't know how to do that, actually. AFAICS
> > rx-offload does not support 'pushing' struct can_frame into the
> > queue, only pulling from rx-offload. There is no fixed connection
> > bewteen the internal can device structure and CAN frames, like
> > hardware message boxes. In order to use
> > can_rx_offload_irq_offload_fifo() to insert (mostly) a single
> > can_frame I would need to store the last urb. I think I need to add a
> > function similar to can_rx_offload_irq_queue_err_skb(), e.g.
> > can_rx_offload_irq_queue_skb, but without scheduling. This should be
> > done once the urb was completly read.
> 
> Does your device support multiple CAN frames per USB?

Yes, it does. Although this will happen rarely actually. As several URBs are 
submitted at once, each URB should only deliver a single CAN frame as this is 
much faster. So in order to receive more CAN frames per URB the USB host (or 
driver) needs to be delayed for long time so the CAN frames queue up on the 
device itself.

> Maybe rename
> 
> > int can_rx_offload_irq_queue_err_skb(struct can_rx_offload *offload,
> > struct sk_buff *skb)
> into can_rx_offload_irq_queue_skb() or
> 
> can_rx_offload_irq_queue_skb_schedule() and fix the race condition:
> > int can_rx_offload_irq_queue_skb(struct can_rx_offload *offload, struct
> > sk_buff *skb) {
> 
> take spinlock
> 
> >         if (skb_queue_len(&offload->skb_queue) >
> >         
> >             offload->skb_queue_len_max)
> 
> release in case of error
> 
> >                 return -ENOMEM;
> >         
> >         skb_queue_tail(&offload->skb_queue, skb);
> 
> use unlocked variant of skb_queue_tail, i.e. __skb_queue_tail()
> 
> release spin lock
> 
> >         can_rx_offload_schedule(offload);
> >         
> >         return 0;
> > 
> > }
> 
> Then factor all but the can_rx_offload_schedule(offload) call into a
> separate function.

I will think about this. I'm afraid but currently I can't do anything about 
that. But this will only work for (USB) CAN drivers though. Ethernet USB 
drivers also suffer from the out-of-order issue as they also push their SKB in 
IRQ context. I've seen reorder warnings in wireshark on a direct link iperf 
transfer.

Best regards,
Alexander
-- 
Dipl.-Inf. Alexander Stein
SYS TEC electronic GmbH
alexander.stein@systec-electronic.com

Legal and Commercial Address:
Am Windrad 2
08468 Heinsdorfergrund
Germany

Office: +49 (0) 3765 38600-0
Fax:    +49 (0) 3765 38600-4100
 
Managing Directors:
	Director Technology/CEO: Dipl.-Phys. Siegmar Schmidt;
	Director Commercial Affairs/COO: Dipl. Ing. (FH) Armin von Collrepp
Commercial Registry:
	Amtsgericht Chemnitz, HRB 28082; USt.-Id Nr. DE150534010