LARTC - Jun 2012 - [PATCH net-next] em_canid: Ematch rule to match CAN frames according to their CAN IDs

This ematch makes it possible to classify CAN frames (AF_CAN) according
to their identifiers. This functionality can not be easily achieved with
existing classifiers, such as u32, because CAN ID is always stored in
native endianness, whereas u32 expects Network byte order.

The filtering rules for EFF frames are stored in an array, which
is traversed during classification. A bitmap is used to store SFF
rules -- one bit for each ID.

It is possible to to pass up to 32 ''rules'' to this ematch
during
configuration.

Signed-off-by: Rostislav Lisovy <lisovy@gmail.com>
---

This Patch contains a reworked classifier initially posted in
http://www.spinics.net/lists/netdev/msg200114.html
The functionality is the same however there is almost 50% reduction
in the source code length.

There were simple benchmark performed on MPC5200 -- an embedded PowerPC CPU
(e300 core, G2 LE), 396 MHz, with 128 MiB of RAM running 3.4.2 Linux kernel
The benchmark simply generated CAN frames with different identifiers and the
time spent in can_send() function was measured.

CAN device was configured as follows:
ip link set can0 type can bitrate 1000000
ip link set can0 txqueuelen 1000

CAN frames were generated with command:
cangen can0 -I ${ID} -L 8 -D i -g 1 -n 100

With no extra filter/qdisc configured, median of the time spent in can_send()
was about 27 us -- with prio qdisc with 5 bands and 5 appropriate cls_can
filters (previous patch), it was about 30 us -- with prio qdisc with 5 bands
and 5 appropriate em_can filters (this patch), it was about 34 us.

---
 include/linux/can.h     |    3 +
 include/linux/pkt_cls.h |    5 +-
 net/sched/Kconfig       |   10 ++
 net/sched/Makefile      |    1 +
 net/sched/em_canid.c    |  252 +++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 269 insertions(+), 2 deletions(-)
 create mode 100644 net/sched/em_canid.c

diff --git a/include/linux/can.h b/include/linux/can.h
index 9a19bcb..08d1610 100644
--- a/include/linux/can.h
+++ b/include/linux/can.h
@@ -38,6 +38,9 @@
  */
 typedef __u32 canid_t;
 
+#define CAN_SFF_ID_BITS		11
+#define CAN_EFF_ID_BITS		29
+
 /*
  * Controller Area Network Error Frame Mask structure
  *
diff --git a/include/linux/pkt_cls.h b/include/linux/pkt_cls.h
index defbde2..7fbe6c2 100644
--- a/include/linux/pkt_cls.h
+++ b/include/linux/pkt_cls.h
@@ -451,8 +451,9 @@ enum {
 #define	TCF_EM_U32		3
 #define	TCF_EM_META		4
 #define	TCF_EM_TEXT		5
-#define        TCF_EM_VLAN		6
-#define	TCF_EM_MAX		6
+#define	TCF_EM_VLAN		6
+#define        TCF_EM_CANID		7
+#define	TCF_EM_MAX		7
 
 enum {
 	TCF_EM_PROG_TC
diff --git a/net/sched/Kconfig b/net/sched/Kconfig
index 75b58f8..bc0ceab 100644
--- a/net/sched/Kconfig
+++ b/net/sched/Kconfig
@@ -485,6 +485,16 @@ config NET_EMATCH_TEXT
 	  To compile this code as a module, choose M here: the
 	  module will be called em_text.
 
+config NET_EMATCH_CANID
+	tristate "CAN ID"
+	depends on NET_EMATCH && CAN
+	---help---
+          Say Y here if you want to be able to classify CAN frames based
+          on CAN ID.
+
+	  To compile this code as a module, choose M here: the
+	  module will be called em_canid.
+
 config NET_CLS_ACT
 	bool "Actions"
 	---help---
diff --git a/net/sched/Makefile b/net/sched/Makefile
index 8cdf4e2..47f9262 100644
--- a/net/sched/Makefile
+++ b/net/sched/Makefile
@@ -53,3 +53,4 @@ obj-$(CONFIG_NET_EMATCH_NBYTE)	+= em_nbyte.o
 obj-$(CONFIG_NET_EMATCH_U32)	+= em_u32.o
 obj-$(CONFIG_NET_EMATCH_META)	+= em_meta.o
 obj-$(CONFIG_NET_EMATCH_TEXT)	+= em_text.o
+obj-$(CONFIG_NET_EMATCH_CANID)	+= em_canid.o
diff --git a/net/sched/em_canid.c b/net/sched/em_canid.c
new file mode 100644
index 0000000..5cc6e5e
--- /dev/null
+++ b/net/sched/em_canid.c
@@ -0,0 +1,252 @@
+/*
+ * em_canid.c  Ematch rule to match CAN frames according to their CAN IDs
+ *
+ *              This program is free software; you can distribute it and/or
+ *              modify it under the terms of the GNU General Public License
+ *              as published by the Free Software Foundation; either version
+ *              2 of the License, or (at your option) any later version.
+ *
+ * Idea:       Oliver Hartkopp <oliver.hartkopp@volkswagen.de>
+ * Copyright:  (c) 2011 Czech Technical University in Prague
+ *             (c) 2011 Volkswagen Group Research
+ * Authors:    Michal Sojka <sojkam1@fel.cvut.cz>
+ *             Pavel Pisa <pisa@cmp.felk.cvut.cz>
+ *             Rostislav Lisovy <lisovy@gmail.cz>
+ * Funded by:  Volkswagen Group Research
+ */
+
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/kernel.h>
+#include <linux/string.h>
+#include <linux/skbuff.h>
+#include <net/pkt_cls.h>
+#include <linux/can.h>
+
+#define EM_CAN_RULES_SIZE				32
+
+struct canid_match {
+	/*
+	 * Raw rules copied from netlink message; Used for sending
+	 * information to userspace (when ''tc filter show'' is
invoked)
+	 * AND when matching EFF frames
+	 */
+	struct can_filter rules_raw[EM_CAN_RULES_SIZE];
+
+	/* For each SFF CAN ID (11 bit) there is one record in this bitfield */
+	DECLARE_BITMAP(match_sff, (1 << CAN_SFF_ID_BITS));
+
+	int rules_count;
+	int eff_rules_count;
+	int sff_rules_count;
+};
+
+/**
+ * em_canid_get_id() - Extracts Can ID out of the sk_buff structure.
+ */
+static canid_t em_canid_get_id(struct sk_buff *skb)
+{
+	/* CAN ID is stored within the data field */
+	struct can_frame *cf = (struct can_frame *)skb->data;
+
+	return cf->can_id;
+}
+
+static void em_canid_sff_match_add(struct canid_match *cm, u32 can_id,
+					u32 can_mask)
+{
+	int i;
+
+	/*
+	 * Limit can_mask and can_id to SFF range to
+	 * protect against write after end of array
+	 */
+	can_mask &= CAN_SFF_MASK;
+	can_id &= can_mask;
+
+	/* Single frame */
+	if (can_mask == CAN_SFF_MASK) {
+		set_bit(can_id, cm->match_sff);
+		return;
+	}
+
+	/* All frames */
+	if (can_mask == 0) {
+		bitmap_fill(cm->match_sff, (1 << CAN_SFF_ID_BITS));
+		return;
+	}
+
+	/*
+	 * Individual frame filter.
+	 * Add record (set bit to 1) for each ID that
+	 * conforms particular rule
+	 */
+	for (i = 0; i < (1 << CAN_SFF_ID_BITS); i++) {
+		if ((i & can_mask) == can_id)
+			set_bit(i, cm->match_sff);
+	}
+}
+
+static inline struct canid_match *em_canid_priv(struct tcf_ematch *m)
+{
+	return (struct canid_match *) (m)->data;
+}
+
+static int em_canid_match(struct sk_buff *skb, struct tcf_ematch *m,
+			 struct tcf_pkt_info *info)
+{
+	struct canid_match *cm = em_canid_priv(m);
+	canid_t can_id;
+	unsigned int match = false;
+	int i;
+
+	can_id = em_canid_get_id(skb);
+
+	if (can_id & CAN_EFF_FLAG) {
+		can_id &= CAN_EFF_MASK;
+
+		for (i = 0; i < cm->eff_rules_count; i++) {
+			if (!(((cm->rules_raw[i].can_id ^ can_id) &
+			    cm->rules_raw[i].can_mask) & CAN_EFF_MASK)) {
+				match = true;
+				break;
+			}
+		}
+	} else { /* SFF */
+		can_id &= CAN_SFF_MASK;
+		match = test_bit(can_id, cm->match_sff);
+	}
+
+	if (match)
+		return 1;
+
+	return 0;
+}
+
+static int em_canid_change(struct tcf_proto *tp, void *data, int len,
+			  struct tcf_ematch *m)
+{
+	struct can_filter *conf = data; /* Array with rules,
+					 * fixed size EM_CAN_RULES_SIZE
+					 */
+	struct canid_match *cm;
+	int err;
+	int i;
+
+	if (len < sizeof(struct can_filter))
+		return -EINVAL;
+
+	err = -ENOBUFS;
+	cm = kzalloc(sizeof(*cm), GFP_KERNEL);
+	if (cm == NULL)
+		goto errout;
+
+	cm->sff_rules_count = 0;
+	cm->eff_rules_count = 0;
+	cm->rules_count = len/sizeof(struct can_filter);
+	err = -EINVAL;
+
+	/* Be sure to fit into the array */
+	if (cm->rules_count > EM_CAN_RULES_SIZE)
+		goto errout_free;
+
+	/*
+	 * We need two for() loops for copying rules into
+	 * two contiguous areas in rules_raw
+	 */
+
+	/* Process EFF frame rules*/
+	for (i = 0; i < cm->rules_count; i++) {
+		if ((conf[i].can_id & CAN_EFF_FLAG) &&
+		    (conf[i].can_mask & CAN_EFF_FLAG)) {
+			memcpy(cm->rules_raw + cm->eff_rules_count,
+				&conf[i],
+				sizeof(struct can_filter));
+
+			cm->eff_rules_count++;
+		} else {
+			continue;
+		}
+	}
+
+	/* Process SFF frame rules */
+	for (i = 0; i < cm->rules_count; i++) {
+		if ((conf[i].can_id & CAN_EFF_FLAG) &&
+		    (conf[i].can_mask & CAN_EFF_FLAG)) {
+			continue;
+		} else {
+			memcpy(cm->rules_raw
+				+ cm->eff_rules_count
+				+ cm->sff_rules_count,
+				&conf[i], sizeof(struct can_filter));
+
+			cm->sff_rules_count++;
+
+			em_canid_sff_match_add(cm,
+				conf[i].can_id, conf[i].can_mask);
+		}
+	}
+
+	m->datalen = sizeof(*cm);
+	m->data = (unsigned long) cm;
+
+	return 0;
+
+errout_free:
+	kfree(cm);
+errout:
+	return err;
+}
+
+static void em_canid_destroy(struct tcf_proto *tp, struct tcf_ematch *m)
+{
+	struct canid_match *cm = em_canid_priv(m);
+
+	kfree(cm);
+}
+
+static int em_canid_dump(struct sk_buff *skb, struct tcf_ematch *m)
+{
+	struct canid_match *cm = em_canid_priv(m);
+
+	/*
+	 * When configuring this ematch ''rules_count'' is set not to
exceed
+	 * ''rules_raw'' array size
+	 */
+	if (nla_put_nohdr(skb, sizeof(cm->rules_raw[0]) * cm->rules_count,
+	    &cm->rules_raw) < 0)
+		goto nla_put_failure;
+
+	return 0;
+
+nla_put_failure:
+	return -1;
+}
+
+static struct tcf_ematch_ops em_canid_ops = {
+	.kind	  = TCF_EM_CANID,
+	.change	  = em_canid_change,
+	.match	  = em_canid_match,
+	.destroy  = em_canid_destroy,
+	.dump	  = em_canid_dump,
+	.owner	  = THIS_MODULE,
+	.link	  = LIST_HEAD_INIT(em_canid_ops.link)
+};
+
+static int __init init_em_canid(void)
+{
+	return tcf_em_register(&em_canid_ops);
+}
+
+static void __exit exit_em_canid(void)
+{
+	tcf_em_unregister(&em_canid_ops);
+}
+
+MODULE_LICENSE("GPL");
+
+module_init(init_em_canid);
+module_exit(exit_em_canid);
+
+MODULE_ALIAS_TCF_EMATCH(TCF_EM_CANID);
-- 
1.7.10

Hello Rostislav,

thanks for the new patch. It got indeed pretty short now :-)

I found some time for a review. See details inline ...

On 18.06.2012 14:22, Rostislav Lisovy wrote:

> 
> With no extra filter/qdisc configured, median of the time spent in
can_send()
> was about 27 us -- with prio qdisc with 5 bands and 5 appropriate cls_can
> filters (previous patch), it was about 30 us -- with prio qdisc with 5
bands
> and 5 appropriate em_can filters (this patch), it was about 34 us.

Hm that''s more than twice the time consumed for classification ...

cls_can: 3 us more
em_can:  7 us more

@Eric: Is this still the better approach then?

> diff --git a/net/sched/Kconfig b/net/sched/Kconfig
> index 75b58f8..bc0ceab 100644
> --- a/net/sched/Kconfig
> +++ b/net/sched/Kconfig
> @@ -485,6 +485,16 @@ config NET_EMATCH_TEXT
>  	  To compile this code as a module, choose M here: the
>  	  module will be called em_text.
>  
> +config NET_EMATCH_CANID
> +	tristate "CAN ID"

i would prefer

tristate "Controller Area Network Identifier"

or at least

tristate "CAN Identifier"
> +	depends on NET_EMATCH && CAN
> +	---help---
> +          Say Y here if you want to be able to classify CAN frames based
> +          on CAN ID.

"on the CAN Identifier."

(..)
> +#include <net/pkt_cls.h>
> +#include <linux/can.h>
> +
> +#define EM_CAN_RULES_SIZE				32

Reduce the gap before ''32'' to one single space.

(..)
> +static int em_canid_match(struct sk_buff *skb, struct tcf_ematch *m,
> +			 struct tcf_pkt_info *info)
> +{
> +	struct canid_match *cm = em_canid_priv(m);
> +	canid_t can_id;
> +	unsigned int match = false;

You use test_bit() below which returns an int.

Better use int instead of unsigned int ...

int match = 0;

> +	int i;
> +
> +	can_id = em_canid_get_id(skb);
> +
> +	if (can_id & CAN_EFF_FLAG) {
> +		can_id &= CAN_EFF_MASK;
> +
> +		for (i = 0; i < cm->eff_rules_count; i++) {
> +			if (!(((cm->rules_raw[i].can_id ^ can_id) &
> +			    cm->rules_raw[i].can_mask) & CAN_EFF_MASK)) {

Looks really tricky. I''m still pondering if it does what it should do
...
> +				match = true;

match = 1;
> +				break;
> +			}
> +		}
> +	} else { /* SFF */
> +		can_id &= CAN_SFF_MASK;
> +		match = test_bit(can_id, cm->match_sff);
> +	}
> +

return match;

> +	if (match)
> +		return 1;
> +
> +	return 0;
> +}
> +
> +static int em_canid_change(struct tcf_proto *tp, void *data, int len,
> +			  struct tcf_ematch *m)
> +{
> +	struct can_filter *conf = data; /* Array with rules,
> +					 * fixed size EM_CAN_RULES_SIZE
> +					 */
> +	struct canid_match *cm;
> +	int err;
> +	int i;
> +
> +	if (len < sizeof(struct can_filter))
> +		return -EINVAL;

if (len % sizeof(struct can_filter))
	return -EINVAL;

if (len > sizeof(struct can_filter) * EM_CAN_RULES_SIZE)
	return -EINVAL;

All checks done before kzalloc() => no need for error cleanups at the end of
the function.
> +
> +	err = -ENOBUFS;

remove
> +	cm = kzalloc(sizeof(*cm), GFP_KERNEL);
> +	if (cm == NULL)

if (!cm)
	return -ENOMEM;
> +		goto errout;

The only user of errout - to be removed.
> +
> +	cm->sff_rules_count = 0;
> +	cm->eff_rules_count = 0;
> +	cm->rules_count = len/sizeof(struct can_filter);
> +	err = -EINVAL;
remove
> +
> +	/* Be sure to fit into the array */
> +	if (cm->rules_count > EM_CAN_RULES_SIZE)
> +		goto errout_free;

already checked before => remove
> +
> +	/*
> +	 * We need two for() loops for copying rules into
> +	 * two contiguous areas in rules_raw
> +	 */
> +
> +	/* Process EFF frame rules*/
> +	for (i = 0; i < cm->rules_count; i++) {
> +		if ((conf[i].can_id & CAN_EFF_FLAG) &&
> +		    (conf[i].can_mask & CAN_EFF_FLAG)) {
> +			memcpy(cm->rules_raw + cm->eff_rules_count,

Oops. Shouldn''t this be

cm->rules_raw + cm->eff_rules_count * sizeof(struct can_filter),

???
> +				&conf[i],
> +				sizeof(struct can_filter));
> +
> +			cm->eff_rules_count++;
> +		} else {
> +			continue;
> +		}

omit { } around continue
> +	}
> +
> +	/* Process SFF frame rules */
> +	for (i = 0; i < cm->rules_count; i++) {
> +		if ((conf[i].can_id & CAN_EFF_FLAG) &&
> +		    (conf[i].can_mask & CAN_EFF_FLAG)) {

What if CAN_EFF_FLAG is set in can_id but not in can_mask ?
> +			continue;
> +		} else {
> +			memcpy(cm->rules_raw
> +				+ cm->eff_rules_count
> +				+ cm->sff_rules_count,

dito

cm->rules_raw + (cm->eff_rules_count + cm->sff_rules_count) *
sizeof(struct
can_filter),

???

> +				&conf[i], sizeof(struct can_filter));
> +
> +			cm->sff_rules_count++;
> +
> +			em_canid_sff_match_add(cm,
> +				conf[i].can_id, conf[i].can_mask);
> +		}
> +	}
> +
> +	m->datalen = sizeof(*cm);
> +	m->data = (unsigned long) cm;
> +
> +	return 0;
> +
> +errout_free:
> +	kfree(cm);
> +errout:
> +	return err;

error handling can be removed with the above changes.
> +}
> +
> +static void em_canid_destroy(struct tcf_proto *tp, struct tcf_ematch *m)
> +{
> +	struct canid_match *cm = em_canid_priv(m);
> +

Check for cm == NULL not needed ?
> +	kfree(cm);
> +}
> +
> +static int em_canid_dump(struct sk_buff *skb, struct tcf_ematch *m)
> +{
> +	struct canid_match *cm = em_canid_priv(m);
> +

Check for cm == NULL not needed ?

Can a dump happen before the matches are added??
> +	/*
> +	 * When configuring this ematch ''rules_count'' is set
not to exceed
> +	 * ''rules_raw'' array size
> +	 */
> +	if (nla_put_nohdr(skb, sizeof(cm->rules_raw[0]) * cm->rules_count,

better sizeof(struct can_filter) instead of sizeof(cm->rules_raw[0]) ??
> +	    &cm->rules_raw) < 0)
> +		goto nla_put_failure;
> +
> +	return 0;
> +
> +nla_put_failure:
> +	return -1;
> +}
> +
> +static struct tcf_ematch_ops em_canid_ops = {
> +	.kind	  = TCF_EM_CANID,
> +	.change	  = em_canid_change,
> +	.match	  = em_canid_match,
> +	.destroy  = em_canid_destroy,
> +	.dump	  = em_canid_dump,
> +	.owner	  = THIS_MODULE,
> +	.link	  = LIST_HEAD_INIT(em_canid_ops.link)
> +};
> +
> +static int __init init_em_canid(void)
> +{
> +	return tcf_em_register(&em_canid_ops);
> +}
> +
> +static void __exit exit_em_canid(void)
> +{
> +	tcf_em_unregister(&em_canid_ops);
> +}
> +
> +MODULE_LICENSE("GPL");
> +
> +module_init(init_em_canid);
> +module_exit(exit_em_canid);
> +
> +MODULE_ALIAS_TCF_EMATCH(TCF_EM_CANID);

Regards,
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-can"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Jun 26, 2012 at 10:07:56PM +0200, Oliver Hartkopp
wrote:
> > With no extra filter/qdisc configured, median of the time spent in
can_send()
> > was about 27 us -- with prio qdisc with 5 bands and 5 appropriate
cls_can
> > filters (previous patch), it was about 30 us -- with prio qdisc with 5
bands
> > and 5 appropriate em_can filters (this patch), it was about 34 us.
> 
> 
> Hm that''s more than twice the time consumed for classification ...
> 
> cls_can: 3 us more
> em_can:  7 us more
> 
> @Eric: Is this still the better approach then?
If there is overhead, we should get rid of that overhead and not
abandon an established subsystem.

Rostislav: Can you provide some details on where the time is spent?
> > +	/* Process EFF frame rules*/
> > +	for (i = 0; i < cm->rules_count; i++) {
> > +		if ((conf[i].can_id & CAN_EFF_FLAG) &&
> > +		    (conf[i].can_mask & CAN_EFF_FLAG)) {
> > +			memcpy(cm->rules_raw + cm->eff_rules_count,
> 
> 
> Oops. Shouldn''t this be
> 
> cm->rules_raw + cm->eff_rules_count * sizeof(struct can_filter),
> 
> ???
Looks like correct pointer arithmetic to me. Your suggestion
would only be valid if rules_raw was a void pointer.
> > +static void em_canid_destroy(struct tcf_proto *tp, struct tcf_ematch
*m)
> > +{
> > +	struct canid_match *cm = em_canid_priv(m);
> > +
> 
> 
> Check for cm == NULL not needed ?
kfree() has that check embeddded. Also, for destroy() can only be called
if the match was added to the tree and that requires a successful call
to ->change(). Therefore it will never be NULL.
> > +static int em_canid_dump(struct sk_buff *skb, struct tcf_ematch *m)
> > +{
> > +	struct canid_match *cm = em_canid_priv(m);
> > +
> 
> 
> Check for cm == NULL not needed ?
> 
> Can a dump happen before the matches are added??
Nope, ->dump() is only ever called if the match has been added to the tree.
> > +	/*
> > +	 * When configuring this ematch ''rules_count'' is
set not to exceed
> > +	 * ''rules_raw'' array size
> > +	 */
> > +	if (nla_put_nohdr(skb, sizeof(cm->rules_raw[0]) *
cm->rules_count,
> 
> 
> better sizeof(struct can_filter) instead of sizeof(cm->rules_raw[0]) ??
> 
> > +	    &cm->rules_raw) < 0)
> > +		goto nla_put_failure;
No need for a goto here, just return -EMSGSIZE.
--
To unsubscribe from this list: send the line "unsubscribe linux-can"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Oliver, Rostislav,

I was just looking into this. I think the matching itself
may be simplified by eliminating checks ''that have already been
made''.

On Tue, Jun 26, 2012 at 10:07:56PM +0200, Oliver Hartkopp
wrote:
> 
> > +static int em_canid_match(struct sk_buff *skb, struct tcf_ematch *m,
> > +			 struct tcf_pkt_info *info)
> > +{
> > +	struct canid_match *cm = em_canid_priv(m);
> > +	canid_t can_id;
> > +	unsigned int match = false;
> > +	int i;
> > +
> > +	can_id = em_canid_get_id(skb);
> > +
> > +	if (can_id & CAN_EFF_FLAG) {
> > +		can_id &= CAN_EFF_MASK;
Why clear the CAN_EFF_FLAG?
It needs an extra read-modify-write, and the CAN_EFF_FLAG is set in
the match rule too.
IMHO, you could leave can_id as it is.
> > +
> > +		for (i = 0; i < cm->eff_rules_count; i++) {
to speed things up manually, I tend to use a ''const struct can_filter
*lp''
and do:
		for (i = 0, lp = cm->rules_raw; i < cm->eff_rules_count;
				++i, ++lp) {
The advantage depends on the compiler''s optimizations, which
I''m not really
aware of.
The next statement would then be:

			if (!((lp->can_id ^ can_id) & lp->can_mask)) {

for stripping & CAN_EFF_MASK, see below :-)

> > +			if (!(((cm->rules_raw[i].can_id ^ can_id) &
> > +			    cm->rules_raw[i].can_mask) & CAN_EFF_MASK)) {
> 
> 
> Looks really tricky. I''m still pondering if it does what it should
do ...
I think it does, since:
	cm->rules_raw[i].can_id ^ can_id
gives an value where the bits that differ are set.
then:
	& cm->rules_raw[i].can_mask
will strip bits that you don''t care.

But ''& CAN_EFF_MASK'' is not needed, since:
	* can_id will have CAN_EFF_FLAG (see comment above)
	* cm->rules_raw[i].can_id has CAN_EFF_FLAG, otherwise it would
	  not be in the list
	* can_id will not have CAN_ERR_FLAG
	* cm->rules_raw should not have CAN_ERR_FLAG
	  you can always clear CAN_ERR_FLAG from the rules during
	  em_canid_change below
	* maybe distinguishing on CAN_RTR_FLAG makes sense during
	  classification.
> 
> > +				match = true;
> > +				break;
> > +			}
> > +		}
> > +	} else { /* SFF */
> > +		can_id &= CAN_SFF_MASK;
> > +		match = test_bit(can_id, cm->match_sff);
> > +	}
> > +
> 
> 
> return match;
> 
> 
> > +}
> > +
--
To unsubscribe from this list: send the line "unsubscribe linux-can"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hello Oliver;

On Tue, 2012-06-26 at 22:07 +0200, Oliver Hartkopp wrote:
> I found some time for a review. See details inline ...
> 
I agree with quite everything except for following...

> > +				match = true;
> 
> 
> match = 1;
> 
egrep -r "= true;" ./linux-source | wc -l
returns 6770 -- why don''t you like "= true"?

> > +				break;
> > +			}
> > +		}
> > +	} else { /* SFF */
> > +		can_id &= CAN_SFF_MASK;
> > +		match = test_bit(can_id, cm->match_sff);
> > +	}
> > +
> 
> 
> return match;
> 
match() function must return 1 or 0, however (from my experience)
test_bit() returns 0 and non-0 (strictly speaking, in my case, 0 and
-1).

> > +				&conf[i],
> > +				sizeof(struct can_filter));
> > +
> > +			cm->eff_rules_count++;
> > +		} else {
> > +			continue;
> > +		}
> 
> 
> omit { } around continue
> 
http://lxr.linux.no/#linux+v3.4.4/Documentation/CodingStyle#L169

> > +	}
> > +
> > +	/* Process SFF frame rules */
> > +	for (i = 0; i < cm->rules_count; i++) {
> > +		if ((conf[i].can_id & CAN_EFF_FLAG) &&
> > +		    (conf[i].can_mask & CAN_EFF_FLAG)) {
> 
> 
> What if CAN_EFF_FLAG is set in can_id but not in can_mask ?
> 
There were small misunderstanding from my side -- this will be rewritten
in the way that EFF_FLAG in mask will determine if we care about the
value of EFF_FLAG in an identifier -- i.e. when EFF_FLAG is set in the
mask, the rule will be added as SFF or EFF only depending on EFF_FLAG
value in the identifier. If EFF_FLAG is 0 in the mask, the rule will be
added as both SFF and EFF.

Regards;
Rostislav

--
To unsubscribe from this list: send the line "unsubscribe linux-can"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, 2012-06-27 at 11:33 +0200, Kurt Van Dijck wrote:
> Oliver, Rostislav,
> 
> I was just looking into this. I think the matching itself
> may be simplified by eliminating checks ''that have already been
made''.
Thank you for your remarks, I have removed all of the unnecessary
CAN_EFF_MASK masking.

Regards;
Rostislav 

--
To unsubscribe from this list: send the line "unsubscribe linux-can"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hello Rostislav,

On 28.06.2012 15:35, Rostislav Lisovy wrote:
> Hello Oliver;
> 
> On Tue, 2012-06-26 at 22:07 +0200, Oliver Hartkopp wrote: 
>> I found some time for a review. See details inline ...
>>
> I agree with quite everything except for following...
> 
> 
>>> +				match = true;
>>
>>
>> match = 1;
>>
> egrep -r "= true;" ./linux-source | wc -l
> returns 6770 -- why don''t you like "= true"?
> 

Just because match is an integer and no boolean.
To me integers contain values - or you assign it to constants which are
usually defined in CAPITAL letters like TRUE and FALSE :-)

You define an int instead of a boolean and use true and false.
It''s just a inconvenient mix to me.
Don''t know.
> 
>>> +				break;
>>> +			}
>>> +		}
>>> +	} else { /* SFF */
>>> +		can_id &= CAN_SFF_MASK;
>>> +		match = test_bit(can_id, cm->match_sff);
>>> +	}
>>> +
>>
>>
>> return match;
>>
> match() function must return 1 or 0, however (from my experience)
> test_bit() returns 0 and non-0 (strictly speaking, in my case, 0 and
> -1).

As you need to return "1" or "0" with your function you
should stick on ''int''
and real values IMO.

What about 

	match = (test_bit(can_id, cm->match_sff))?1:0;

and working with "1" and "0" all the time instead of the
final correction to
the return values at the end of the function?

Like this (based on the latest version in your git):

diff --git a/net/sched/em_canid.c b/net/sched/em_canid.c
index f0c7c75..9e4b705 100644
--- a/net/sched/em_canid.c
+++ b/net/sched/em_canid.c
@@ -98,7 +98,7 @@ static int em_canid_match(struct sk_buff *skb, struct
tcf_ematch *m,
 {
        struct canid_match *cm = em_canid_priv(m);
        canid_t can_id;
-       int match = false;
+       int match = 0;
        int i;
        const struct can_filter *lp;
 
@@ -108,19 +108,16 @@ static int em_canid_match(struct sk_buff *skb, struct
tcf_ematch *m,
                for (i = 0, lp = cm->rules_raw;
                     i < cm->eff_rules_count; i++, lp++) {
                        if (!(((lp->can_id ^ can_id) & lp->can_mask)))
{
-                               match = true;
+                               match = 1;
                                break;
                        }
                }
        } else { /* SFF */
                can_id &= CAN_SFF_MASK;
-               match = test_bit(can_id, cm->match_sff);
+               match = (test_bit(can_id, cm->match_sff))?1:0;
        }
 
-       if (match)
-               return 1;
-
-       return 0;
+       return match;
 }
 
 static int em_canid_change(struct tcf_proto *tp, void *data, int len,


It''s nitpicking - but the if statement to tweak the return value from
test_bit() is only performed in the test_bit() case.
> 
> 
>>> +				&conf[i],
>>> +				sizeof(struct can_filter));
>>> +
>>> +			cm->eff_rules_count++;
>>> +		} else {
>>> +			continue;
>>> +		}
>>
>>
>> omit { } around continue
>>
> http://lxr.linux.no/#linux+v3.4.4/Documentation/CodingStyle#L169
> 

ok.
> 
>>> +	}
>>> +
>>> +	/* Process SFF frame rules */
>>> +	for (i = 0; i < cm->rules_count; i++) {
>>> +		if ((conf[i].can_id & CAN_EFF_FLAG) &&
>>> +		    (conf[i].can_mask & CAN_EFF_FLAG)) {
>>
>>
>> What if CAN_EFF_FLAG is set in can_id but not in can_mask ?
>>
> There were small misunderstanding from my side -- this will be rewritten
> in the way that EFF_FLAG in mask will determine if we care about the
> value of EFF_FLAG in an identifier -- i.e. when EFF_FLAG is set in the
> mask, the rule will be added as SFF or EFF only depending on EFF_FLAG
> value in the identifier. If EFF_FLAG is 0 in the mask, the rule will be
> added as both SFF and EFF.
> 

Great.

Tnx,
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-can"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, 2012-06-28 at 18:35 +0200, Oliver Hartkopp wrote:
> 
> What about 
> 
> 	match = (test_bit(can_id, cm->match_sff))?1:0;
> 
> and working with "1" and "0" all the time instead of
the final correction to
> the return values at the end of the function?
I agree;

Regards;
Rostislav

--
To unsubscribe from this list: send the line "unsubscribe linux-can"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html