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

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 5429 bytes --]

On 03/29/2015 08:09 PM, Oliver Hartkopp wrote:
> 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 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 <socketcan@hartkopp.net>
> ---
>  Documentation/networking/can.txt | 20 ++++++++++++++++++--
>  include/uapi/linux/can/raw.h     |  1 +
>  net/can/raw.c                    | 36 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 55 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/networking/can.txt b/Documentation/networking/can.txt
> index 0a2859a..5abad1e 100644
> --- a/Documentation/networking/can.txt
> +++ b/Documentation/networking/can.txt
> @@ -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:
>  
> diff --git a/include/uapi/linux/can/raw.h b/include/uapi/linux/can/raw.h
> index 78ec76f..8735f108 100644
> --- a/include/uapi/linux/can/raw.h
> +++ b/include/uapi/linux/can/raw.h
> @@ -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 */
> diff --git a/net/can/raw.c b/net/can/raw.c
> index 866a9b3..dc68fa0 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -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)

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?)

> +				return;
> +		} else
>  			return;

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

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

-- 
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: 819 bytes --]