Skip to contentSkip to conversation

can: join filters with per-CPU variables

RFC PATCH In review Oliver Hartkopp 2 participants 6 messages Apr 1, 2015

Subsystems (5)
lore
Share & tools
lore

Oliver Hartkopp

author maintainer
#0 lore raw

v3 cover can: join filters with per-CPU variables

Hi all,

as Andre Naujoks found some problems in my first patches with multi-threading I did some investigation how to handle variables to be written inside the NET_RX softirq without locking. As far as I was able to test this code it works properly now - even with the multi-thread test from Andre.

If you know how to use per-CPU variables inside NET_RX softirq I would appreciate your review as I'm not that sure that the per-CPU code is correct. It's my first time using this technique and is only what I was able to get from other code examples and available kernel docs.

Many thanks,
Oliver

Changes v1 -> v2:
* Introduce per-CPU variables

Changes v2 -> v3:
* Fix style issues remarked by Sergei Shtylyov and Marc Kleine-Budde
* Indroduced a per-CPU struct as suggested by Marc Kleine-Budde
* omitted the obsolete per-CPU struct data initialization as alloc_percpu provides already zero'ed memory

Oliver Hartkopp (2):
can: fix multiple delivery of a single CAN frame for overlapping CAN
filters
can: introduce new raw socket option to join the given CAN filters

 Documentation/networking/can.txt | 20 ++++++++++++++--
 include/uapi/linux/can/raw.h     |  1 +
 net/can/raw.c                    | 50 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+), 2 deletions(-)
Signature · 1 line
2.1.4
author maintainer
#1 lore raw

RFC PATCH v3 1/2 can: fix multiple delivery of a single CAN frame for overlapping CAN filters

The CAN_RAW socket can set multiple CAN identifier specific filters that lead to multiple filters in the af_can.c filter processing. These filters are indenpendent from each other which leads to logical OR'ed filters when applied.

This patch makes sure that every CAN frame which is filtered for a specific socket is only delivered once to the user space. This is independent from the number of matching CAN filters of this socket.

As the raw_rcv() function is executed from NET_RX softirq the introduced variables are implemented as per-CPU variables to avoid extensive locking at CAN frame reception time.

---
 net/can/raw.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
net/can/raw.c +21 -0
index 00c13ef..8c75446 100644
@@ -74,6 +74,11 @@ MODULE_ALIAS("can-proto-1");
  * storing the single filter in dfilter, to avoid using dynamic memory.
  */
 
+struct uniqframe {
+	const struct sk_buff *skb;
+	ktime_t tstamp;
+};
+
 struct raw_sock {
 	struct sock sk;
 	int bound;
@@ -86,6 +91,7 @@ struct raw_sock {
 	struct can_filter dfilter; /* default/single filter */
 	struct can_filter *filter; /* pointer to filter(s) */
 	can_err_mask_t err_mask;
+	struct uniqframe __percpu *uniq;
 };
 
 /*
@@ -123,6 +129,15 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
 	if (!ro->fd_frames && oskb->len != CAN_MTU)
 		return;
 
+	/* 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)) {
+		return;
+	} else {
+		this_cpu_ptr(ro->uniq)->skb = oskb;
+		this_cpu_ptr(ro->uniq)->tstamp = oskb->tstamp;
+	}
+
 	/* clone the given skb to be able to enqueue it into the rcv queue */
 	skb = skb_clone(oskb, GFP_ATOMIC);
 	if (!skb)
@@ -297,6 +312,11 @@ static int raw_init(struct sock *sk)
 	ro->recv_own_msgs    = 0;
 	ro->fd_frames        = 0;
 
+	/* alloc_percpu provides zero'ed memory */
+	ro->uniq = alloc_percpu(struct uniqframe);
+	if (unlikely(!ro->uniq))
+		return -ENOMEM;
+
 	/* set notifier */
 	ro->notifier.notifier_call = raw_notifier;
 
@@ -339,6 +359,7 @@ static int raw_release(struct socket *sock)
 	ro->ifindex = 0;
 	ro->bound   = 0;
 	ro->count   = 0;
+	free_percpu(ro->uniq);
 
 	sock_orphan(sk);
 	sock->sk = NULL;
Signature · 1 line
2.1.4
#2 lore raw

Re: [RFC PATCH v3 1/2] can: fix multiple delivery of a single CAN frame for overlapping CAN filters

Quoted 22 lines · Oliver Hartkopp

The CAN_RAW socket can set multiple CAN identifier specific filters that lead to multiple filters in the af_can.c filter processing. These filters are indenpendent from each other which leads to logical OR'ed filters when applied.

This patch makes sure that every CAN frame which is filtered for a specific socket is only delivered once to the user space. This is independent from the number of matching CAN filters of this socket.

As the raw_rcv() function is executed from NET_RX softirq the introduced variables are implemented as per-CPU variables to avoid extensive locking at CAN frame reception time.

---
 net/can/raw.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
net/can/raw.c +0 -0
index 00c13ef..8c75446 100644

net/can/raw.c MODULE_ALIAS("can-proto-1"); · line 80

Quoted 7 lines
net/can/raw.c +4 -0
@@ -74,6 +74,11 @@ MODULE_ALIAS("can-proto-1");
  * storing the single filter in dfilter, to avoid using dynamic memory.
  */
 
+struct uniqframe {
+	const struct sk_buff *skb;
+	ktime_t tstamp;
+};

Nitpick: as ktime_t is always a 64 bit value, I think putting it first might provide better alignment.

No need to repost, I'll change this while applying, if you're okay with this.

Marc

Signature · 4 lines
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   |
author maintainer
#3 lore raw

Re: [RFC PATCH v3 1/2] can: fix multiple delivery of a single CAN frame for overlapping CAN filters

Hi Marc,

Quoted 8 lines · Marc Kleine-Budde

net/can/raw.c MODULE_ALIAS("can-proto-1"); · line 80

Quoted 4 lines · Oliver Hartkopp
net/can/raw.c +4 -0
+struct uniqframe {
+	const struct sk_buff *skb;
+	ktime_t tstamp;
+};

Nitpick: as ktime_t is always a 64 bit value, I think putting it first might provide better alignment.

No objections. Good idea.

Quoted 3 lines

No need to repost, I'll change this while applying, if you're okay with this.

I would suggest to take both patches into can-next (for 4.1).

Even though I did some testing on my i7 machine I would like this per-CPU stuff to come to maturity with some more testing of other users.

IMO the patch 1/2 (aka 'the fix') is not critical - at least it was not for the last 10 years :-)

So if $COSTUMER really needs this functionality it should be no problem to apply these really short and clear patches.

Best regards,
Oliver

#4 lore raw

Re: [RFC PATCH v3 1/2] can: fix multiple delivery of a single CAN frame for overlapping CAN filters

Quoted 10 lines · Oliver Hartkopp
Quoted 8 lines · Oliver Hartkopp

net/can/raw.c MODULE_ALIAS("can-proto-1"); · line 80

Quoted 4 lines
net/can/raw.c +4 -0
+struct uniqframe {
+	const struct sk_buff *skb;
+	ktime_t tstamp;
+};

Nitpick: as ktime_t is always a 64 bit value, I think putting it first might provide better alignment.

No objections. Good idea.

Thanks.

Quoted 4 lines
Quoted 2 lines

No need to repost, I'll change this while applying, if you're okay with this.

I would suggest to take both patches into can-next (for 4.1).

I'll create a pull-request asap.

Marc.

Signature · 4 lines
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   |
author maintainer
#5 lore raw

RFC PATCH v3 2/2 can: introduce new raw socket option to join the given CAN filters

In reply to Oliver Hartkopp

The CAN_RAW socket can set multiple CAN identifier specific filters that lead to multiple filters in the af_can.c filter processing. These filters are indenpendent from each other which leads to logical OR'ed filters when applied.

This socket option joines the given CAN filters in the way that only CAN frames are passed to user space that matched *all* given CAN filters. The semantic for the applied filters is therefore changed to a logical AND.

This is useful especially when the filterset is a combination of filters where the CAN_INV_FILTER flag is set in order to notch single CAN IDs or CAN ID ranges from the incoming traffic.

As the raw_rcv() function is executed from NET_RX softirq the introduced variables are implemented as per-CPU variables to avoid extensive locking at CAN frame reception time.

---
 Documentation/networking/can.txt | 20 ++++++++++++++++++--
 include/uapi/linux/can/raw.h     |  1 +
 net/can/raw.c                    | 31 ++++++++++++++++++++++++++++++-
 3 files changed, 49 insertions(+), 3 deletions(-)
Documentation/networking/can.txt +18 -2
index 0a2859a..5abad1e 100644
@@ -22,7 +22,8 @@ This file contains
       4.1.3 RAW socket option CAN_RAW_LOOPBACK
       4.1.4 RAW socket option CAN_RAW_RECV_OWN_MSGS
       4.1.5 RAW socket option CAN_RAW_FD_FRAMES
-      4.1.6 RAW socket returned message flags
+      4.1.6 RAW socket option CAN_RAW_JOIN_FILTERS
+      4.1.7 RAW socket returned message flags
     4.2 Broadcast Manager protocol sockets (SOCK_DGRAM)
       4.2.1 Broadcast Manager operations
       4.2.2 Broadcast Manager message flags
@@ -601,7 +602,22 @@ solution for a couple of reasons:
   CAN FD frames by checking if the device maximum transfer unit is CANFD_MTU.
   The CAN device MTU can be retrieved e.g. with a SIOCGIFMTU ioctl() syscall.
 
-  4.1.6 RAW socket returned message flags
+  4.1.6 RAW socket option CAN_RAW_JOIN_FILTERS
+
+  The CAN_RAW socket can set multiple CAN identifier specific filters that
+  lead to multiple filters in the af_can.c filter processing. These filters
+  are indenpendent from each other which leads to logical OR'ed filters when
+  applied (see 4.1.1).
+
+  This socket option joines the given CAN filters in the way that only CAN
+  frames are passed to user space that matched *all* given CAN filters. The
+  semantic for the applied filters is therefore changed to a logical AND.
+
+  This is useful especially when the filterset is a combination of filters
+  where the CAN_INV_FILTER flag is set in order to notch single CAN IDs or
+  CAN ID ranges from the incoming traffic.
+
+  4.1.7 RAW socket returned message flags
 
   When using recvmsg() call, the msg->msg_flags may contain following flags:
 
include/uapi/linux/can/raw.h +1 -0
index 78ec76f..8735f108 100644
@@ -57,6 +57,7 @@ enum {
 	CAN_RAW_LOOPBACK,	/* local loopback (default:on)       */
 	CAN_RAW_RECV_OWN_MSGS,	/* receive my own msgs (default:off) */
 	CAN_RAW_FD_FRAMES,	/* allow CAN FD frames (default:off) */
+	CAN_RAW_JOIN_FILTERS,	/* all filters must match to trigger */
 };
 
 #endif /* !_UAPI_CAN_RAW_H */
net/can/raw.c +30 -1
index 8c75446..7b94554 100644
@@ -77,6 +77,7 @@ MODULE_ALIAS("can-proto-1");
 struct uniqframe {
 	const struct sk_buff *skb;
 	ktime_t tstamp;
+	unsigned int join_rx_count;
 };
 
 struct raw_sock {
@@ -87,6 +88,7 @@ struct raw_sock {
 	int loopback;
 	int recv_own_msgs;
 	int fd_frames;
+	int join_filters;
 	int count;                 /* number of active filters */
 	struct can_filter dfilter; /* default/single filter */
 	struct can_filter *filter; /* pointer to filter(s) */
@@ -132,10 +134,21 @@ 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)) {
-		return;
+		if (ro->join_filters) {
+			this_cpu_inc(ro->uniq->join_rx_count);
+			/* drop frame until all enabled filters matched */
+			if (this_cpu_ptr(ro->uniq)->join_rx_count < ro->count)
+				return;
+		} else {
+			return;
+		}
 	} else {
 		this_cpu_ptr(ro->uniq)->skb = oskb;
 		this_cpu_ptr(ro->uniq)->tstamp = oskb->tstamp;
+		this_cpu_ptr(ro->uniq)->join_rx_count = 1;
+		/* drop first frame to check all enabled filters? */
+		if (ro->join_filters && ro->count > 1)
+			return;
 	}
 
 	/* clone the given skb to be able to enqueue it into the rcv queue */
@@ -311,6 +324,7 @@ static int raw_init(struct sock *sk)
 	ro->loopback         = 1;
 	ro->recv_own_msgs    = 0;
 	ro->fd_frames        = 0;
+	ro->join_filters     = 0;
 
 	/* alloc_percpu provides zero'ed memory */
 	ro->uniq = alloc_percpu(struct uniqframe);
@@ -604,6 +618,15 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 
 		break;
 
+	case CAN_RAW_JOIN_FILTERS:
+		if (optlen != sizeof(ro->join_filters))
+			return -EINVAL;
+
+		if (copy_from_user(&ro->join_filters, optval, optlen))
+			return -EFAULT;
+
+		break;
+
 	default:
 		return -ENOPROTOOPT;
 	}
@@ -668,6 +691,12 @@ static int raw_getsockopt(struct socket *sock, int level, int optname,
 		val = &ro->fd_frames;
 		break;
 
+	case CAN_RAW_JOIN_FILTERS:
+		if (len > sizeof(int))
+			len = sizeof(int);
+		val = &ro->join_filters;
+		break;
+
 	default:
 		return -ENOPROTOOPT;
 	}
Signature · 1 line
2.1.4