can: join filters with per-CPU variables

RFC PATCH Superseded Oliver Hartkopp 3 participants 17 messages Mar 29, 2015 – Mar 31, 2015

Subsystems (5)

Networking Network drivers Documentation CAN network drivers +1 more subsystems

lore

Share & tools

lore

Oliver Hartkopp

author maintainer

2015-03-29 18:09 UTC #0 lore raw

v2 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

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                    | 64 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 83 insertions(+), 2 deletions(-)

Signature · 1 line

2.1.4

Oliver Hartkopp

author maintainer

2015-03-29 18:09 UTC #1 lore raw

RFC PATCH v2 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 can_raw() 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.

Signed-off-by Oliver Hartkopp <[email protected]>

---
 net/can/raw.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

net/can/raw.c +28 -0

index 00c13ef..866a9b3 100644
@@ -86,6 +86,8 @@ 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 sk_buff __percpu **uniq_skb;
+	ktime_t __percpu *uniq_tstamp;
 };
 
 /*

@@ -123,6 +125,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)

@@ -282,6 +293,7 @@ static int raw_notifier(struct notifier_block *nb,
 static int raw_init(struct sock *sk)
 {
 	struct raw_sock *ro = raw_sk(sk);
+	int cpu;
 
 	ro->bound            = 0;
 	ro->ifindex          = 0;

@@ -297,6 +309,20 @@ static int raw_init(struct sock *sk)
 	ro->recv_own_msgs    = 0;
 	ro->fd_frames        = 0;
 
+	ro->uniq_skb = alloc_percpu(struct sk_buff *);
+	if (unlikely(ro->uniq_skb == NULL))
+		return -ENOMEM;
+	for_each_possible_cpu(cpu)
+		*per_cpu_ptr(ro->uniq_skb, cpu) = NULL;
+
+	ro->uniq_tstamp = alloc_percpu(ktime_t);
+	if (unlikely(ro->uniq_tstamp == NULL)) {
+		free_percpu(ro->uniq_skb);
+		return -ENOMEM;
+	}
+	for_each_possible_cpu(cpu)
+		*per_cpu_ptr(ro->uniq_tstamp, cpu) = ktime_set(0, 0);
+
 	/* set notifier */
 	ro->notifier.notifier_call = raw_notifier;

@@ -339,6 +365,8 @@ static int raw_release(struct socket *sock)
 	ro->ifindex = 0;
 	ro->bound   = 0;
 	ro->count   = 0;
+	free_percpu(ro->uniq_skb);
+	free_percpu(ro->uniq_tstamp);
 
 	sock_orphan(sk);
 	sock->sk = NULL;

Signature · 1 line

2.1.4

Marc Kleine-Budde

maintainer

2015-03-30 09:50 UTC #2 lore raw

Re: [PATCH RFC v2 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 can_raw() 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.
Signed-off-by Oliver Hartkopp <[email protected]>
---
 net/can/raw.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
net/can/raw.c +0 -0
index 00c13ef..866a9b3 100644

net/can/raw.c struct raw_sock { · line 90

Quoted 5 lines

net/can/raw.c +2 -0

@@ -86,6 +86,8 @@ 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 sk_buff __percpu **uniq_skb;
+	ktime_t __percpu *uniq_tstamp;

What about creating a struct to hold the ktime_t and the skb? BTW: the pointer to the skb can be marked as const.

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   |

Oliver Hartkopp

author maintainer

2015-03-30 10:29 UTC #3 lore raw

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

Quoted 6 lines · Marc Kleine-Budde

net/can/raw.c struct raw_sock { · line 90
Quoted 3 lines
can_err_mask_t err_mask;
net/can/raw.c +2 -0
+	struct sk_buff __percpu **uniq_skb;
+	ktime_t __percpu *uniq_tstamp;
What about creating a struct to hold the ktime_t and the skb? BTW: the pointer to the skb can be marked as const.

Don't think so.

The pointer points to some allocated memory.

Regards,
Oliver

Marc Kleine-Budde

maintainer

2015-03-30 10:36 UTC #4 lore raw

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

Quoted 9 lines · Oliver Hartkopp

Quoted 6 lines · Marc Kleine-Budde
net/can/raw.c struct raw_sock { · line 90
Quoted 3 lines
can_err_mask_t err_mask;
net/can/raw.c +2 -0
+	struct sk_buff __percpu **uniq_skb;
+	ktime_t __percpu *uniq_tstamp;
What about creating a struct to hold the ktime_t and the skb? BTW: the pointer to the skb can be marked as const.

Quoted 3 lines

Don't think so.
The pointer points to some allocated memory.

Yes, but the allocated memory points to a skb which is not modified, so it's a:

struct sk_buff __percpu * const *uniq_skb;

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   |

Marc Kleine-Budde

maintainer

2015-03-30 10:10 UTC #5 lore raw

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

In reply to Oliver Hartkopp

Quoted 33 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 can_raw() 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.
Signed-off-by Oliver Hartkopp <[email protected]>
---
 net/can/raw.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
net/can/raw.c +2 -0
index 00c13ef..866a9b3 100644
@@ -86,6 +86,8 @@ 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 sk_buff __percpu **uniq_skb;
+	ktime_t __percpu *uniq_tstamp;
 };
 
 /*
@@ -123,6 +125,15 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
 	if (!ro->fd_frames && oskb->len != CAN_MTU)
 		return;

net/can/raw.c static void raw_rcv(struct sk_buff *oskb, void *data) · line 136

Quoted 10 lines

net/can/raw.c +9 -0

 
+	/* 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;
+	}
+

What happens if you're preempted somewhere in this code, it's not atomic? I think, if we only have to take care about the skb, an atomic compare exchange would work. But we have two variables....If you use a struct (see previous mail), I think the usage of get_cpu_ptr(), git_cpu_ptr() ensures that we're not preempted.

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   |

Marc Kleine-Budde

maintainer

2015-03-30 10:16 UTC #6 lore raw

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

Quoted 4 lines · Marc Kleine-Budde

Quoted 3 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.

independent

Quoted 10 lines

Quoted 9 lines
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 can_raw() 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.
Signed-off-by Oliver Hartkopp <[email protected]>

Quoted 15 lines

net/can/raw.c static void raw_rcv(struct sk_buff *oskb, void *data) · line 136
Quoted 9 lines
net/can/raw.c +9 -0
+	/* 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;
+	}
+
What happens if you're preempted somewhere in this code, it's not atomic? I think, if we only have to take care about the skb, an atomic compare exchange would work. But we have two variables....If you use a struct (see previous mail), I think the usage of get_cpu_ptr(), git_cpu_ptr() ensures that we're not preempted.

^^^^^^^^^^^^^
put_cpu_ptr()

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   |

Oliver Hartkopp

author maintainer

2015-03-30 10:41 UTC #7 lore raw

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

In reply to Marc Kleine-Budde

Quoted 17 lines · Marc Kleine-Budde

net/can/raw.c static void raw_rcv(struct sk_buff *oskb, void *data) · line 136
Quoted 10 lines
net/can/raw.c +9 -0
+	/* 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;
+	}
+
What happens if you're preempted somewhere in this code, it's not atomic? I think, if we only have to take care about the skb, an atomic compare exchange would work. But we have two variables....If you use a struct (see previous mail), I think the usage of get_cpu_ptr(), git_cpu_ptr() ensures that we're not preempted.

Please check out

https://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/

And especially https://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/x173.html#LOCK-SOFTIRQS-SAME

When a softirq processes an incoming skb this remains on that selected CPU.

The mutithread-test from Andre just lead to the problem that the (former single instance) variables ro->uniq_skb and ro->uniq_tstamp have been used by different CPUs which made the checks unreliable.

So following the documentation and other examples in kernel source you can

- use spinlocks in can_receive() in af_can.c (instead of rcu_read_lock())
- use per-CPU variables to allow the softirq to run in parallel

Just make the variables atomic (as you suggested) is as bad as introduce spinlocks in can_receive() as you reduce the skb processing to just one thread. So at least percpu is the best for performance but needs to create a vector of variables (percpu).

Putting a struct into these percpu handling can be done - but does it increase the readability in this case?

Regards,
Oliver

Marc Kleine-Budde

maintainer

2015-03-31 12:32 UTC #8 lore raw

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

Quoted 20 lines · Oliver Hartkopp

Quoted 17 lines · Marc Kleine-Budde
net/can/raw.c static void raw_rcv(struct sk_buff *oskb, void *data) · line 136
Quoted 10 lines
net/can/raw.c +9 -0
+	/* 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;
+	}
+
What happens if you're preempted somewhere in this code, it's not atomic? I think, if we only have to take care about the skb, an atomic compare exchange would work. But we have two variables....If you use a struct (see previous mail), I think the usage of get_cpu_ptr(), git_cpu_ptr() ensures that we're not preempted.

Quoted 8 lines

Please check out
https://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/
And especially
https://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/x173.html#LOCK-SOFTIRQS-SAME
When a softirq processes an incoming skb this remains on that selected CPU.

Okay, I was not sure about this. What about preempt_rt?

Quoted 6 lines

The mutithread-test from Andre just lead to the problem that the (former single instance) variables ro->uniq_skb and ro->uniq_tstamp have been used by different CPUs which made the checks unreliable.
So following the documentation and other examples in kernel source you can

Quoted 7 lines

- use spinlocks in can_receive() in af_can.c (instead of rcu_read_lock())
- use per-CPU variables to allow the softirq to run in parallel
Just make the variables atomic (as you suggested) is as bad as introduce spinlocks in can_receive() as you reduce the skb processing to just one thread. So at least percpu is the best for performance but needs to create a vector of variables (percpu).

Ack, lockless atomic-compare-exchange is only possbile for a single variable.

Quoted 2 lines

Putting a struct into these percpu handling can be done - but does it increase the readability in this case?

It saves ressources, 1 pointer instead of 3 (considering both of your patches) and only 1 allocation.

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   |

Oliver Hartkopp

author maintainer

2015-03-31 20:24 UTC #9 lore raw

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

Quoted 12 lines · Marc Kleine-Budde

Quoted 8 lines · Oliver Hartkopp
Please check out
https://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/
And especially
https://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/x173.html#LOCK-SOFTIRQS-SAME
When a softirq processes an incoming skb this remains on that selected CPU.
Okay, I was not sure about this. What about preempt_rt?

I don't care :-)

There are so many implementations that rely on this per-CPU stuff that we can assume preempt_rt takes care of it (e.g. with locking or reducing CPU cores, etc)

Quoted 5 lines

Quoted 2 lines
Putting a struct into these percpu handling can be done - but does it increase the readability in this case?
It saves ressources, 1 pointer instead of 3 (considering both of your patches) and only 1 allocation.

Yes. I did some more code reading, created a struct for it and omitted the initialization as alloc_percpu returns an already zero'ed memory region.

Will send tomorrow morning.

Regards,
Oliver

Sergei Shtylyov

2015-03-30 12:33 UTC #10 lore raw

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

In reply to Oliver Hartkopp

Hello.

Quoted 21 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 can_raw() 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.
Signed-off-by Oliver Hartkopp <[email protected]>
---
  net/can/raw.c | 28 ++++++++++++++++++++++++++++
  1 file changed, 28 insertions(+)
net/can/raw.c +0 -0
index 00c13ef..866a9b3 100644

[...]

net/can/raw.c static void raw_rcv(struct sk_buff *oskb, void *data) · line 131

Quoted 8 lines

net/can/raw.c +4 -0

@@ -123,6 +125,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;

Over-indented.

Quoted 8 lines

net/can/raw.c +5 -0

+	} 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)

[...]

net/can/raw.c static int raw_init(struct sock *sk) · line 319

Quoted 12 lines

net/can/raw.c +8 -0

@@ -297,6 +309,20 @@ static int raw_init(struct sock *sk)
  	ro->recv_own_msgs    = 0;
  	ro->fd_frames        = 0;
 
+	ro->uniq_skb = alloc_percpu(struct sk_buff *);
+	if (unlikely(ro->uniq_skb == NULL))
+		return -ENOMEM;
+	for_each_possible_cpu(cpu)
+		*per_cpu_ptr(ro->uniq_skb, cpu) = NULL;
+
+	ro->uniq_tstamp = alloc_percpu(ktime_t);
+	if (unlikely(ro->uniq_tstamp == NULL)) {

!ro->uniq_tstamp is preferred in the networking code.

[...]

WBR, Sergei

Oliver Hartkopp

author maintainer

2015-03-30 15:49 UTC #11 lore raw

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

Quoted 9 lines · Sergei Shtylyov

net/can/raw.c static void raw_rcv(struct sk_buff *oskb, void *data) · line 131
Quoted 5 lines · Oliver Hartkopp
net/can/raw.c +4 -0
+    /* 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;
Over-indented.

I was asked about that before. AFAIK the *skb is no unique identifier over a longer period of time. But together with the timestamp it becomes unique. Or do you have a better solution to detect identical skbs?

CAN skbs do not have a (rx)hash so far and I wonder if it's worth to compute the hash in favor to check the *skb and the timestamp here ...

Quoted 6 lines

net/can/raw.c static int raw_init(struct sock *sk) · line 319
Quoted 3 lines
net/can/raw.c +3 -0
+
+    ro->uniq_tstamp = alloc_percpu(ktime_t);
+    if (unlikely(ro->uniq_tstamp == NULL)) {
    !ro->uniq_tstamp is preferred in the networking code.

Ok. Will change that.

Best regards,
Oliver

Quoted 4 lines

[...]
WBR, Sergei

Sergei Shtylyov

2015-03-30 17:14 UTC #12 lore raw

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

Hello.

Quoted 10 lines · Oliver Hartkopp

Quoted 6 lines
net/can/raw.c static void raw_rcv(struct sk_buff *oskb, void *data) · line 131
Quoted 4 lines
net/can/raw.c +4 -0
+    /* 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;
Over-indented.
I was asked about that before. AFAIK the *skb is no unique identifier over a longer period of time. But together with the timestamp it becomes unique. Or do you have a better solution to detect identical skbs?

I just said that *return* was too far to the right, that's all. :-)

Quoted 2 lines

CAN skbs do not have a (rx)hash so far and I wonder if it's worth to compute the hash in favor to check the *skb and the timestamp here ...

[...]

Quoted 2 lines

Best regards,
Oliver

WBR, Sergei

Oliver Hartkopp

author maintainer

2015-03-30 17:25 UTC #13 lore raw

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

Quoted 7 lines · Sergei Shtylyov

Quoted 4 lines · Oliver Hartkopp

Quoted 4 lines

net/can/raw.c +4 -0

+    /* 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;

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
Over-indented.
I was asked about that before. AFAIK the *skb is no unique identifier over a longer period of time. But together with the timestamp it becomes unique. Or do you have a better solution to detect identical skbs?
I just said that *return* was too far to the right, that's all. :-)

Oh, ok ...

I mixed indented with intended %-)

My bad.

Will fix that in v3 too.

Good thing: So far no one complained about the __percpu stuff ...

Many thanks,
Oliver

Oliver Hartkopp

author maintainer

2015-03-29 18:09 UTC #14 lore raw

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

In reply to Oliver Hartkopp

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 can_raw() 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.

Signed-off-by Oliver Hartkopp <[email protected]>

---
 Documentation/networking/can.txt | 20 ++++++++++++++++++--
 include/uapi/linux/can/raw.h     |  1 +
 net/can/raw.c                    | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 2 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 +36 -0

index 866a9b3..dc68fa0 100644
@@ -82,6 +82,8 @@ struct raw_sock {
 	int loopback;
 	int recv_own_msgs;
 	int fd_frames;
+	int join_filters;
+	int __percpu *join_rx_count;
 	int count;                 /* number of active filters */
 	struct can_filter dfilter; /* default/single filter */
 	struct can_filter *filter; /* pointer to filter(s) */

@@ -128,10 +130,20 @@ 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)) {
+		if (ro->join_filters) {
+			this_cpu_inc(*ro->join_rx_count);
+			/* drop frame until all enabled filters matched */
+			if (*this_cpu_ptr(ro->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->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 */

@@ -308,6 +320,7 @@ static int raw_init(struct sock *sk)
 	ro->loopback         = 1;
 	ro->recv_own_msgs    = 0;
 	ro->fd_frames        = 0;
+	ro->join_filters     = 0;
 
 	ro->uniq_skb = alloc_percpu(struct sk_buff *);
 	if (unlikely(ro->uniq_skb == NULL))

@@ -323,6 +336,13 @@ static int raw_init(struct sock *sk)
 	for_each_possible_cpu(cpu)
 		*per_cpu_ptr(ro->uniq_tstamp, cpu) = ktime_set(0, 0);
 
+	ro->join_rx_count = alloc_percpu(int);
+	if (unlikely(ro->join_rx_count == NULL)) {
+		free_percpu(ro->uniq_skb);
+		free_percpu(ro->uniq_tstamp);
+		return -ENOMEM;
+	}
+
 	/* set notifier */
 	ro->notifier.notifier_call = raw_notifier;

@@ -367,6 +387,7 @@ static int raw_release(struct socket *sock)
 	ro->count   = 0;
 	free_percpu(ro->uniq_skb);
 	free_percpu(ro->uniq_tstamp);
+	free_percpu(ro->join_rx_count);
 
 	sock_orphan(sk);
 	sock->sk = NULL;

@@ -611,6 +632,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;
 	}

@@ -675,6 +705,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

Marc Kleine-Budde

maintainer

2015-03-31 12:36 UTC #15 lore raw

Re: [PATCH RFC v2 2/2] can: introduce new raw socket option to join the given CAN filters

Quoted 87 lines · Oliver Hartkopp

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 can_raw() 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.

Signed-off-by Oliver Hartkopp <[email protected]>

---
 Documentation/networking/can.txt | 20 ++++++++++++++++++--
 include/uapi/linux/can/raw.h     |  1 +
 net/can/raw.c                    | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 2 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 +2 -0

index 866a9b3..dc68fa0 100644
@@ -82,6 +82,8 @@ struct raw_sock {
 	int loopback;
 	int recv_own_msgs;
 	int fd_frames;
+	int join_filters;
+	int __percpu *join_rx_count;
 	int count;                 /* number of active filters */
 	struct can_filter dfilter; /* default/single filter */
 	struct can_filter *filter; /* pointer to filter(s) */

net/can/raw.c static void raw_rcv(struct sk_buff *oskb, void *data) · line 136

Quoted 7 lines

net/can/raw.c +4 -0

@@ -128,10 +130,20 @@ 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)) {
+		if (ro->join_filters) {
+			this_cpu_inc(*ro->join_rx_count);
+			/* drop frame until all enabled filters matched */
+			if (*this_cpu_ptr(ro->join_rx_count) < ro->count)

Can we be sure, that all skbs are processed on the same CPU? So that it's sufficient to just compare the "ro->join_rx_count" of this CPU with "ro->count" (or do we have to use join_rx_count of all CPUs?)

net/can/raw.c static void raw_rcv(struct sk_buff *oskb, void *data) · line 139

Quoted 3 lines

net/can/raw.c +2 -0

+				return;
+		} else
 			return;

nitpick: please use { } on both sides of the else.

net/can/raw.c static void raw_rcv(struct sk_buff *oskb, void *data) · line 147

Quoted 8 lines

net/can/raw.c +4 -0

 	} else {
 		*this_cpu_ptr(ro->uniq_skb) = oskb;
 		*this_cpu_ptr(ro->uniq_tstamp) = oskb->tstamp;
+		*this_cpu_ptr(ro->join_rx_count) = 1;
+		/* drop first frame to check all enabled filters? */
+		if (ro->join_filters && ro->count > 1)
+			return;
 	}

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   |

Oliver Hartkopp

author maintainer

2015-03-31 20:30 UTC #16 lore raw

Re: [PATCH RFC v2 2/2] can: introduce new raw socket option to join the given CAN filters

Quoted 16 lines · Marc Kleine-Budde

net/can/raw.c static void raw_rcv(struct sk_buff *oskb, void *data) · line 136
Quoted 8 lines · Oliver Hartkopp
net/can/raw.c +4 -0
@@ -128,10 +130,20 @@ 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)) {
+		if (ro->join_filters) {
+			this_cpu_inc(*ro->join_rx_count);
+			/* drop frame until all enabled filters matched */
+			if (*this_cpu_ptr(ro->join_rx_count) < ro->count)
Can we be sure, that all skbs are processed on the same CPU? So that it's sufficient to just compare the "ro->join_rx_count" of this CPU with "ro->count" (or do we have to use join_rx_count of all CPUs?)

No, that's exactly the point that the single CAN frame skb is processed inside the softirq by one CPU. And we count only for this specific skb here.

The next frame which is processed in raw_rcv() might use another CPU while executing the NET_RX softirq.

Quoted 6 lines

net/can/raw.c static void raw_rcv(struct sk_buff *oskb, void *data) · line 139
Quoted 3 lines
net/can/raw.c +2 -0
+				return;
+		} else
  			return;
nitpick: please use { } on both sides of the else.

fixed in v3

Regards,
Oliver