As reported by Manfred Schlaegl here
http://marc.info/?l=linux-netdev&m=143482089824232&w=2
commit 514ac99c64b "can: fix multiple delivery of a single CAN frame for
overlapping CAN filters" requires the skb->tstamp to be set to check for
identical CAN skbs.
As net timestamping is influenced by several players (netstamp_needed and
netdev_tstamp_prequeue) Manfred missed a proper timestamp which leads to
CAN frame loss.
As skb timestamping became now mandatory for CAN related skbs this patch
makes sure that received CAN skbs always have a proper timestamp set.
Maybe there's a better solution in the future but this patch fixes the
CAN frame loss so far.
Reported-by: Manfred Schlaegl <[email protected]>
Signed-off-by: Oliver Hartkopp <[email protected]>
---
drivers/net/can/dev.c | 5 +++++
drivers/net/can/slcan.c | 1 +
drivers/net/can/vcan.c | 3 +++
net/can/af_can.c | 6 +++++-
4 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index b0f6924..e9b1810 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -440,6 +440,9 @@ unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx)
struct can_frame *cf = (struct can_frame *)skb->data;
u8 dlc = cf->can_dlc;
+ if (!(skb->tstamp.tv64))
+ __net_timestamp(skb);
+
netif_rx(priv->echo_skb[idx]);
priv->echo_skb[idx] = NULL;
@@ -575,6 +578,7 @@ struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf)
if (unlikely(!skb))
return NULL;
+ __net_timestamp(skb);
skb->protocol = htons(ETH_P_CAN);
skb->pkt_type = PACKET_BROADCAST;
skb->ip_summed = CHECKSUM_UNNECESSARY;
@@ -603,6 +607,7 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
if (unlikely(!skb))
return NULL;
+ __net_timestamp(skb);
skb->protocol = htons(ETH_P_CANFD);
skb->pkt_type = PACKET_BROADCAST;
skb->ip_summed = CHECKSUM_UNNECESSARY;
diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index c837eb9..f64f529 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -207,6 +207,7 @@ static void slc_bump(struct slcan *sl)
if (!skb)
return;
+ __net_timestamp(skb);
skb->dev = sl->dev;
skb->protocol = htons(ETH_P_CAN);
skb->pkt_type = PACKET_BROADCAST;
diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c
index 674f367..0ce868d 100644
--- a/drivers/net/can/vcan.c
+++ b/drivers/net/can/vcan.c
@@ -78,6 +78,9 @@ static void vcan_rx(struct sk_buff *skb, struct net_device *dev)
skb->dev = dev;
skb->ip_summed = CHECKSUM_UNNECESSARY;
+ if (!(skb->tstamp.tv64))
+ __net_timestamp(skb);
+
netif_rx_ni(skb);
}
diff --git a/net/can/af_can.c b/net/can/af_can.c
index 32d710e..689c818 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -310,8 +310,12 @@ int can_send(struct sk_buff *skb, int loop)
return err;
}
- if (newskb)
+ if (newskb) {
+ if (!(newskb->tstamp.tv64))
+ __net_timestamp(newskb);
+
netif_rx_ni(newskb);
+ }
/* update statistics */
can_stats.tx_frames++;
--
2.1.4
On 06/21/2015 06:50 PM, Oliver Hartkopp wrote:
> As reported by Manfred Schlaegl here
>
> http://marc.info/?l=linux-netdev&m=143482089824232&w=2
>
> commit 514ac99c64b "can: fix multiple delivery of a single CAN frame for
> overlapping CAN filters" requires the skb->tstamp to be set to check for
> identical CAN skbs.
>
> As net timestamping is influenced by several players (netstamp_needed and
> netdev_tstamp_prequeue) Manfred missed a proper timestamp which leads to
> CAN frame loss.
>
> As skb timestamping became now mandatory for CAN related skbs this patch
> makes sure that received CAN skbs always have a proper timestamp set.
> Maybe there's a better solution in the future but this patch fixes the
> CAN frame loss so far.
>
> Reported-by: Manfred Schlaegl <[email protected]>
> Signed-off-by: Oliver Hartkopp <[email protected]>
I'll send a pullrequest to David asap, I'll add stable on Cc if this
doesn't make it into v4.1 anymore.
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 |
On 21.06.2015 18:57, Marc Kleine-Budde wrote:
>
> I'll send a pullrequest to David asap, I'll add stable on Cc if this
> doesn't make it into v4.1 anymore.
Thanks Marc!
I sent it to netdev and Dave directly too as he was working on his backlog -
and I was not sure if you are available until the next pull request for 4.1.
Best regards,
Oliver
Hello!
On 2015-06-21 18:50, Oliver Hartkopp wrote:
> As reported by Manfred Schlaegl here
>
> http://marc.info/?l=linux-netdev&m=143482089824232&w=2
>
> commit 514ac99c64b "can: fix multiple delivery of a single CAN frame for
> overlapping CAN filters" requires the skb->tstamp to be set to check for
> identical CAN skbs.
>
> As net timestamping is influenced by several players (netstamp_needed and
> netdev_tstamp_prequeue) Manfred missed a proper timestamp which leads to
> CAN frame loss.
>
> As skb timestamping became now mandatory for CAN related skbs this patch
> makes sure that received CAN skbs always have a proper timestamp set.
> Maybe there's a better solution in the future but this patch fixes the
> CAN frame loss so far.
>
I'm not sure, but maybe this patch (and also my original one) opens a new potential issue with timestamps.
If the timestamp is set at allocation time, this cancels setting the timestamp at delivery (by net_timestamp_check in, for example, netif_receive_skb_internal.) -> So it changes the behavior of timestamping (http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/timestamping.txt?id=b953c0d234bc72e8489d3bf51a276c5c4ec85345) generally.
Hypothetical example: If timestamping is enabled by the user and there is a significant delay between allocation and delivery of a skb (early allocation in driver or something) the timestamp does not reflect the reception time anymore.
What do you thing about this?
best regards,
Manfred
Hello Manfred,
On 22.06.2015 12:10, Manfred Schlaegl wrote:
> On 2015-06-21 18:50, Oliver Hartkopp wrote:
>> As reported by Manfred Schlaegl here
>>
>> http://marc.info/?l=linux-netdev&m=143482089824232&w=2
>>
>> commit 514ac99c64b "can: fix multiple delivery of a single CAN frame for
>> overlapping CAN filters" requires the skb->tstamp to be set to check for
>> identical CAN skbs.
>>
>> As net timestamping is influenced by several players (netstamp_needed and
>> netdev_tstamp_prequeue) Manfred missed a proper timestamp which leads to
>> CAN frame loss.
>>
>> As skb timestamping became now mandatory for CAN related skbs this patch
>> makes sure that received CAN skbs always have a proper timestamp set.
>> Maybe there's a better solution in the future but this patch fixes the
>> CAN frame loss so far.
>>
>
> I'm not sure, but maybe this patch (and also my original one) opens a new potential issue with timestamps.
>
> If the timestamp is set at allocation time, this cancels setting the timestamp at delivery (by net_timestamp_check in, for example, netif_receive_skb_internal.) -> So it changes the behavior of timestamping (http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/timestamping.txt?id=b953c0d234bc72e8489d3bf51a276c5c4ec85345) generally.
The only change is that the timestamps for CAN skbs are generated always.
The idea behind the timestamping by user control is to omit the timestamping
when it's not needed. There's no user visible change in behaviour when the
timestamp is set in the CAN skbs all time.
> Hypothetical example: If timestamping is enabled by the user and there is a significant delay between allocation and delivery of a skb (early allocation in driver or something) the timestamp does not reflect the reception time anymore.
The change only affects CAN skbs.
These skbs are allocated at CAN frame reception time, filled with content and
then sent to the network layer.
AFAICS the timestamp becomes more precise for CAN related skbs.
I did not see any case of 'early allocation' in linux/drivers/net/can, did you?
Best regards,
Oliver
Hello Oliver,
On 2015-06-22 12:34, Oliver Hartkopp wrote:
> Hello Manfred,
>
> On 22.06.2015 12:10, Manfred Schlaegl wrote:
>> On 2015-06-21 18:50, Oliver Hartkopp wrote:
>>> As reported by Manfred Schlaegl here
>>>
>>> http://marc.info/?l=linux-netdev&m=143482089824232&w=2
>>>
>>> commit 514ac99c64b "can: fix multiple delivery of a single CAN frame for
>>> overlapping CAN filters" requires the skb->tstamp to be set to check for
>>> identical CAN skbs.
>>>
>>> As net timestamping is influenced by several players (netstamp_needed and
>>> netdev_tstamp_prequeue) Manfred missed a proper timestamp which leads to
>>> CAN frame loss.
>>>
>>> As skb timestamping became now mandatory for CAN related skbs this patch
>>> makes sure that received CAN skbs always have a proper timestamp set.
>>> Maybe there's a better solution in the future but this patch fixes the
>>> CAN frame loss so far.
>>>
>>
>> I'm not sure, but maybe this patch (and also my original one) opens a new potential issue with timestamps.
>>
>> If the timestamp is set at allocation time, this cancels setting the timestamp at delivery (by net_timestamp_check in, for example, netif_receive_skb_internal.) -> So it changes the behavior of timestamping (http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/timestamping.txt?id=b953c0d234bc72e8489d3bf51a276c5c4ec85345) generally.
>
> The only change is that the timestamps for CAN skbs are generated always.
> The idea behind the timestamping by user control is to omit the timestamping when it's not needed. There's no user visible change in behaviour when the timestamp is set in the CAN skbs all time.
>
>> Hypothetical example: If timestamping is enabled by the user and there is a significant delay between allocation and delivery of a skb (early allocation in driver or something) the timestamp does not reflect the reception time anymore.
>
> The change only affects CAN skbs.
> These skbs are allocated at CAN frame reception time, filled with content and then sent to the network layer.
>
> AFAICS the timestamp becomes more precise for CAN related skbs.
> I did not see any case of 'early allocation' in linux/drivers/net/can, did you?
No, I also did not find this case in current driver implementations -- because of that I gave the hypothetical example.
I just was worried about that this may be a potential latent issue for future driver implementations and wanted to indicate this.
But I trust your expertise, so if you are fine with it, I'm too. ;-)
Best regards,
Manfred
On 22.06.2015 13:49, Manfred Schlaegl wrote:
> On 2015-06-22 12:34, Oliver Hartkopp wrote:
>> On 22.06.2015 12:10, Manfred Schlaegl wrote:
>>> Hypothetical example: If timestamping is enabled by the user and there is a significant delay between allocation and delivery of a skb (early allocation in driver or something) the timestamp does not reflect the reception time anymore.
>>
>> The change only affects CAN skbs.
>> These skbs are allocated at CAN frame reception time, filled with content and then sent to the network layer.
>>
>> AFAICS the timestamp becomes more precise for CAN related skbs.
>> I did not see any case of 'early allocation' in linux/drivers/net/can, did you?
>
> No, I also did not find this case in current driver implementations -- because of that I gave the hypothetical example.
> I just was worried about that this may be a potential latent issue for future driver implementations and wanted to indicate this.
>
> But I trust your expertise, so if you are fine with it, I'm too. ;-)
I don't claim to be 'an expert' :-)
But our usual use-case is CAN logging which enables the timestamping on all
CAN interfaces anyway - without any problems.
As the timestamp is calculated only once this patch moves the timestamp
creation closer to the CAN frame arrival time - so latency finally decreases.
Best regards,
Oliver