From: Manfred Schlaegl <manfred.schlaegl@gmx.at>
To: Oliver Hartkopp <socketcan@hartkopp.net>, linux-can@vger.kernel.org
Cc: tom_usenet@optusnet.com.au, s.grosjean@peak-system.com
Subject: Re: [PATCH RFC] can: replace timestamp as unique skb attribute
Date: Mon, 13 Jul 2015 11:26:09 +0200 [thread overview]
Message-ID: <55A38431.9040004@gmx.at> (raw)
In-Reply-To: <1435312699-25999-1-git-send-email-socketcan@hartkopp.net>
[-- Attachment #1: Type: text/plain, Size: 8265 bytes --]
On 2015-06-26 11:58, Oliver Hartkopp wrote:
> 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.
>
> Without timestamping to be required by user space applications this timestamp
> was not generated which lead to commit 36c01245eb8 "can: fix loss of CAN
> frames in raw_rcv" - which forces the timestamp to be set in all CAN related
> skbuffs by introducing several __net_timestamp() calls.
>
> This forces e.g. out of tree drivers to add __net_timestamp() after skbuff
> creation to prevent the frame loss fixed in mainline Linux.
>
> This patch removes the timestamp dependency and uses an atomic counter to
> create an unique identifier together with the skbuff pointer.
>
> Btw. the new skbcnt element introduced in struct can_skb_priv has to be
> initialized with zero in out-of-tree drivers too.
>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
> drivers/net/can/dev.c | 7 ++-----
> drivers/net/can/slcan.c | 2 +-
> drivers/net/can/vcan.c | 3 ---
> include/linux/can/skb.h | 2 ++
> net/can/af_can.c | 12 +++++++-----
> net/can/bcm.c | 2 ++
> net/can/raw.c | 7 ++++---
> 7 files changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index e9b1810..aede704 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -440,9 +440,6 @@ 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;
>
> @@ -578,7 +575,6 @@ 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;
> @@ -589,6 +585,7 @@ struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf)
>
> can_skb_reserve(skb);
> can_skb_prv(skb)->ifindex = dev->ifindex;
> + can_skb_prv(skb)->skbcnt = 0;
>
> *cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame));
> memset(*cf, 0, sizeof(struct can_frame));
> @@ -607,7 +604,6 @@ 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;
> @@ -618,6 +614,7 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
>
> can_skb_reserve(skb);
> can_skb_prv(skb)->ifindex = dev->ifindex;
> + can_skb_prv(skb)->skbcnt = 0;
>
> *cfd = (struct canfd_frame *)skb_put(skb, sizeof(struct canfd_frame));
> memset(*cfd, 0, sizeof(struct canfd_frame));
> diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
> index f64f529..a23a7af 100644
> --- a/drivers/net/can/slcan.c
> +++ b/drivers/net/can/slcan.c
> @@ -207,7 +207,6 @@ 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;
> @@ -215,6 +214,7 @@ static void slc_bump(struct slcan *sl)
>
> can_skb_reserve(skb);
> can_skb_prv(skb)->ifindex = sl->dev->ifindex;
> + can_skb_prv(skb)->skbcnt = 0;
>
> memcpy(skb_put(skb, sizeof(struct can_frame)),
> &cf, sizeof(struct can_frame));
> diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c
> index 0ce868d..674f367 100644
> --- a/drivers/net/can/vcan.c
> +++ b/drivers/net/can/vcan.c
> @@ -78,9 +78,6 @@ 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/include/linux/can/skb.h b/include/linux/can/skb.h
> index b6a52a4..51bb653 100644
> --- a/include/linux/can/skb.h
> +++ b/include/linux/can/skb.h
> @@ -27,10 +27,12 @@
> /**
> * struct can_skb_priv - private additional data inside CAN sk_buffs
> * @ifindex: ifindex of the first interface the CAN frame appeared on
> + * @skbcnt: atomic counter to have an unique id together with skb pointer
> * @cf: align to the following CAN frame at skb->data
> */
> struct can_skb_priv {
> int ifindex;
> + int skbcnt;
> struct can_frame cf[0];
> };
>
> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index 689c818..62c635f 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -89,6 +89,8 @@ struct timer_list can_stattimer; /* timer for statistics update */
> struct s_stats can_stats; /* packet statistics */
> struct s_pstats can_pstats; /* receive list statistics */
>
> +static atomic_t skbcounter = ATOMIC_INIT(0);
> +
> /*
> * af_can socket functions
> */
> @@ -310,12 +312,8 @@ int can_send(struct sk_buff *skb, int loop)
> return err;
> }
>
> - if (newskb) {
> - if (!(newskb->tstamp.tv64))
> - __net_timestamp(newskb);
> -
> + if (newskb)
> netif_rx_ni(newskb);
> - }
>
> /* update statistics */
> can_stats.tx_frames++;
> @@ -683,6 +681,10 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)
> can_stats.rx_frames++;
> can_stats.rx_frames_delta++;
>
> + /* create non-zero unique skb identifier together with *skb */
> + while (!(can_skb_prv(skb)->skbcnt))
> + can_skb_prv(skb)->skbcnt = atomic_inc_return(&skbcounter);
> +
> rcu_read_lock();
>
> /* deliver the packet to sockets listening on all devices */
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index b523453..a1ba687 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -261,6 +261,7 @@ static void bcm_can_tx(struct bcm_op *op)
>
> can_skb_reserve(skb);
> can_skb_prv(skb)->ifindex = dev->ifindex;
> + can_skb_prv(skb)->skbcnt = 0;
>
> memcpy(skb_put(skb, CFSIZ), cf, CFSIZ);
>
> @@ -1217,6 +1218,7 @@ static int bcm_tx_send(struct msghdr *msg, int ifindex, struct sock *sk)
> }
>
> can_skb_prv(skb)->ifindex = dev->ifindex;
> + can_skb_prv(skb)->skbcnt = 0;
> skb->dev = dev;
> can_skb_set_owner(skb, sk);
> err = can_send(skb, 1); /* send with loopback */
> diff --git a/net/can/raw.c b/net/can/raw.c
> index 31b9748..2e67b14 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -75,7 +75,7 @@ MODULE_ALIAS("can-proto-1");
> */
>
> struct uniqframe {
> - ktime_t tstamp;
> + int skbcnt;
> const struct sk_buff *skb;
> unsigned int join_rx_count;
> };
> @@ -133,7 +133,7 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
>
> /* eliminate multiple filter matches for the same skb */
> if (this_cpu_ptr(ro->uniq)->skb == oskb &&
> - ktime_equal(this_cpu_ptr(ro->uniq)->tstamp, oskb->tstamp)) {
> + this_cpu_ptr(ro->uniq)->skbcnt == can_skb_prv(oskb)->skbcnt) {
> if (ro->join_filters) {
> this_cpu_inc(ro->uniq->join_rx_count);
> /* drop frame until all enabled filters matched */
> @@ -144,7 +144,7 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
> }
> } else {
> this_cpu_ptr(ro->uniq)->skb = oskb;
> - this_cpu_ptr(ro->uniq)->tstamp = oskb->tstamp;
> + this_cpu_ptr(ro->uniq)->skbcnt = can_skb_prv(oskb)->skbcnt;
> this_cpu_ptr(ro->uniq)->join_rx_count = 1;
> /* drop first frame to check all enabled filters? */
> if (ro->join_filters && ro->count > 1)
> @@ -749,6 +749,7 @@ static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>
> can_skb_reserve(skb);
> can_skb_prv(skb)->ifindex = dev->ifindex;
> + can_skb_prv(skb)->skbcnt = 0;
>
> err = memcpy_from_msg(skb_put(skb, size), msg, size);
> if (err < 0)
>
I've tested this in my original situation on i.MX6Q so I can give a
Tested-by: Manfred Schlaegl <manfred.schlaegl@gmx.at>
best regards,
manfred
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
next prev parent reply other threads:[~2015-07-13 9:26 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-26 9:58 [PATCH RFC] can: replace timestamp as unique skb attribute Oliver Hartkopp
2015-07-01 8:01 ` Marc Kleine-Budde
2015-07-04 16:00 ` Oliver Hartkopp
2015-07-13 9:26 ` Manfred Schlaegl [this message]
2015-07-13 9:38 ` Marc Kleine-Budde
2015-07-13 9:40 ` Manfred Schlaegl
2015-07-13 9:42 ` Marc Kleine-Budde
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55A38431.9040004@gmx.at \
--to=manfred.schlaegl@gmx.at \
--cc=linux-can@vger.kernel.org \
--cc=s.grosjean@peak-system.com \
--cc=socketcan@hartkopp.net \
--cc=tom_usenet@optusnet.com.au \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).