Fri, Apr 24, 2015 at 07:28:05PM CEST, t...@herbertland.com wrote:
>Hi Jiri,
>
>Thanks for this work, I think it's a good direction! Some comments below...
>
>On Fri, Apr 24, 2015 at 8:51 AM, Jiri Pirko <j...@resnulli.us> wrote:
...
>> enum flow_dissector_key_id {
>> FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */
>> FLOW_DISSECTOR_KEY_IPV4_ADDRS, /* struct flow_dissector_key_addrs */
>> FLOW_DISSECTOR_KEY_IPV6_HASH_ADDRS, /* struct
>> flow_dissector_key_addrs */
>> FLOW_DISSECTOR_KEY_PORTS, /* struct flow_dissector_key_ports */
>> + FLOW_DISSECTOR_KEY_IPV6_ADDRS, /* struct
>> flow_dissector_key_ipv6_addrs */
>>
>And we'll want to add VLAN ID, (GRE) key-id, IPv6 flow label, maybe a
>couple more.
Definitely. I plan to extend keys for those you mentioned and more in
future. Easy extendability is the goal of this patchset.
>
>> FLOW_DISSECTOR_KEY_MAX,
>> };
>> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>> index 564288e..95e9a21 100644
>> --- a/net/core/flow_dissector.c
>> +++ b/net/core/flow_dissector.c
>> @@ -175,16 +175,29 @@ ipv6:
>> ip_proto = iph->nexthdr;
>> nhoff += sizeof(struct ipv6hdr);
>>
>> - if (!skb_flow_dissector_uses_key(flow_dissector,
>> -
>> FLOW_DISSECTOR_KEY_IPV6_HASH_ADDRS))
>> - break;
>> - key_addrs = skb_flow_dissector_target(flow_dissector,
>> -
>> FLOW_DISSECTOR_KEY_IPV6_HASH_ADDRS,
>> - target_container);
>> + if (skb_flow_dissector_uses_key(flow_dissector,
>> +
>> FLOW_DISSECTOR_KEY_IPV6_HASH_ADDRS)) {
>> + key_addrs = skb_flow_dissector_target(flow_dissector,
>> +
>> FLOW_DISSECTOR_KEY_IPV6_HASH_ADDRS,
>> +
>> target_container);
>>
>> - key_addrs->src = (__force __be32)ipv6_addr_hash(&iph->saddr);
>> - key_addrs->dst = (__force __be32)ipv6_addr_hash(&iph->daddr);
>> + key_addrs->src = (__force
>> __be32)ipv6_addr_hash(&iph->saddr);
>> + key_addrs->dst = (__force
>> __be32)ipv6_addr_hash(&iph->daddr);
>> + goto flow_label;
>> + }
>
>So this is still folding the IPv6 addresses so that that we can fit
>into jhash_3words? Can we address this now and include the full IPv6
>address in the hash? I would propose that we extend the flow_keys
>structure (which I think you may already be doing) for full IPv6
>address, VLAN, flow label, etc., and then produce a hash across that
>whole structure. jhash2 can be used on a structure. jhash is actually
>a very efficient hash performance wise-- the rest of flow dissection
>is likely the dominant cost anyway. We should also minimize calls to
>flow_dissector, I would propose it should be called at most once per
>packet-- I'll be reposting patches to fix that in the various qdiscs.
This hash computation is currently present in kernel. I just ported that
to new programable dissector framework.
I'm not sure how possible is the flow_keys structure extension. In this
patchset, I just maintain the current size and usage. But the thing is
that every individual caller of flow_dissect can use their own struct
which includes exactly what he needs.
Regarding the flow_dissect call reduction, I agree that it is something
we need. The question is how to pass the dissect results through the
code. For example, sch_choke uses qdisc_cb_private for passing flow_keys
struct. However qdisc_skb_cb can by only 20 bytes long (QDISC_CB_PRIV_LEN)
This can be however resolver independently of this patchset.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
