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 lartc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
