Re: [RFC] meta ematch

On Fri, 2005-01-14 at 10:14, Thomas Graf wrote:
 

diff -Nru linux-2.6.10-bk14.orig/include/linux/tc_ematch/tc_em_meta.h linux-2.6.10-bk14/include/linux/tc_ematch/tc_em_meta.h

[..]

+struct tcf_meta_val
+{
+	__u16			kind;
+	__u8			shift;
+	__u8			op;
+};
+
+#define TCF_META_TYPE_MASK	(0xf << 12)
+#define TCF_META_TYPE(kind)	(((kind) & TCF_META_TYPE_MASK) >> 12)
+#define TCF_META_ID_MASK	0x7ff
+#define TCF_META_ID(kind)	((kind) & TCF_META_ID_MASK)

Does it smell like there may be endianess issues? Probably not.

+enum
+{
+	TCF_META_TYPE_VAR,
+	TCF_META_TYPE_INT,
+	__TCF_META_TYPE_MAX
+};
+#define TCF_META_TYPE_MAX (__TCF_META_TYPE_MAX - 1)
+
+enum
+{
+	TCF_META_ID_VALUE,
+	TCF_META_ID_RANDOM,
+	TCF_META_ID_LOADAVG_0,
+	TCF_META_ID_LOADAVG_1,
+	TCF_META_ID_LOADAVG_2,
+	TCF_META_ID_DEV,

Since filters are attached to devices - is TCF_META_ID_DEV of any value?

diff -Nru linux-2.6.10-bk14.orig/net/sched/em_meta.c linux-2.6.10-bk14/net/sched/em_meta.c

+
+struct meta_obj
+{
+	unsigned long		value;
+	unsigned int		len;
+};
+
+struct meta_value
+{
+	struct tcf_meta_val	hdr;
+	unsigned long		val;
+	unsigned int		len;

Those last two look like meta_obj you defined above

+/**************************************************************************
+ * System status & misc
+ **************************************************************************/
+
+static int meta_int_random(struct sk_buff *skb, struct tcf_pkt_info *info,
+			   struct meta_value *v, struct meta_obj *dst)
+{
+	get_random_bytes(&dst->value, sizeof(dst->value));
+	return 0;
+}
+
+static inline unsigned long fixed_loadavg(unsigned long v)
+{
+	return (v + (FIXED_1/200)) >> FSHIFT;

200 has some magic connotation to it - a define somewhere perhaps?

+}
+
+static int meta_int_loadavg_0(struct sk_buff *skb, struct tcf_pkt_info *info,
+			      struct meta_value *v, struct meta_obj *dst)
+{
+	dst->value = fixed_loadavg(avenrun[0]);
+	return 0;
+}
+
+static int meta_int_loadavg_1(struct sk_buff *skb, struct tcf_pkt_info *info,
+			      struct meta_value *v, struct meta_obj *dst)
+{
+	dst->value = fixed_loadavg(avenrun[1]);
+	return 0;
+}
+
+static int meta_int_loadavg_2(struct sk_buff *skb, struct tcf_pkt_info *info,
+			      struct meta_value *v, struct meta_obj *dst)
+{
+	dst->value = fixed_loadavg(avenrun[2]);
+	return 0;
+}

Theres a lot of parameters not used at all in these calls .get calls. So
far i have seen dst->value and some of the skb fields used. I apologize,
I normally dont pick on these things - so if you have future plans for
why you are passing those, keep them and ignore the comment.
BTW, it would probably be useful to return some mnemonic instead of 0.

+/**************************************************************************
+ * Device names & indices
+ **************************************************************************/
+
+static inline int int_dev(struct net_device *dev, struct meta_obj *dst)
+{
+	if (unlikely(dev == NULL))
+		return -1;
+
+	dst->value = dev->ifindex;
+	return 0;
+}
+
+static inline int var_dev(struct net_device *dev, struct meta_obj *dst)
+{
+	if (unlikely(dev == NULL))
+		return -1;
+
+	dst->value = (unsigned long) dev->name;
+	dst->len = strlen(dev->name);

So if device dissapears ... what happens to the pointer?
[..]

+static inline struct meta_ops * meta_ops(struct meta_value *v)
+{
+	return &__meta_ops[meta_type(v)][meta_id(v)];
+}
+
+static int meta_var_compare(struct meta_obj *a, struct meta_obj *b)
+{
+	int r = a->len - b->len;
+
+	if (r == 0)
+		r = memcmp((void *) a->value, (void *) b->value, a->len);
+
+	return r;
+}

clever

+static void meta_var_apply_extras(struct meta_value *v,
+				  struct meta_obj *dst)
+{
+	int shift = v->hdr.shift;
+
+	if (shift && shift < dst->len)
+		dst->len -= shift;
+}

whats the purpose to the extras?

+static int meta_int_compare(struct meta_obj *a, struct meta_obj *b)
+{
+	/* Let gcc optimize it, the unlikely is not really based on
+	 * some numbers but jump free code for missmatches seems
+	 * more logical.
+	 */
+	if (unlikely(a == b))
+		return 0;
+	else if (a < b)
+		return -1;
+	else
+		return 1;
+}

Would be very useful to return mnemonics for readability.

+static int em_meta_match(struct sk_buff *skb, struct tcf_ematch *m,
+			 struct tcf_pkt_info *info)
+{
+	int r;
+	struct meta_match *meta = (struct meta_match *) m->data;
+	struct meta_obj l_value, r_value;
+
+	if (meta_get(skb, info, &meta->lvalue, &l_value) < 0 ||
+	    meta_get(skb, info, &meta->rvalue, &r_value) < 0)
+		return 0;

This is one part that confused me in my earlier email

+	r = meta_type_ops(&meta->lvalue)->compare(&l_value, &r_value);

And this is where it started 

Overall comment: Well done
usability comment:
Ok, I have to admit I am not a friend of too-friendly, which is one of
the faults IMO with netfilter; however, this would have
joenetfilterfireman sweat a little too profusely.
I think you could add a new metafield in 31 seconds. I could probably do
it in 95 seconds. Would be ideal to get average
janeorjoenetfilterfireman to do it in 30 minutes - I dont think you are
there.

cheers,
jamal