Messages in this thread | | | From | Matteo Croce <> | Date | Fri, 25 Oct 2019 02:27:28 +0200 | Subject | Re: [PATCH net-next 3/4] flow_dissector: extract more ICMP information |
| |
On Wed, Oct 23, 2019 at 7:55 PM Simon Horman <simon.horman@netronome.com> wrote: > > On Wed, Oct 23, 2019 at 12:53:37PM +0200, Matteo Croce wrote: > > On Wed, Oct 23, 2019 at 12:00 PM Simon Horman > > <simon.horman@netronome.com> wrote: > > > On Mon, Oct 21, 2019 at 10:09:47PM +0200, Matteo Croce wrote: > > > > + switch (ih->type) { > > > > + case ICMP_ECHO: > > > > + case ICMP_ECHOREPLY: > > > > + case ICMP_TIMESTAMP: > > > > + case ICMP_TIMESTAMPREPLY: > > > > + case ICMPV6_ECHO_REQUEST: > > > > + case ICMPV6_ECHO_REPLY: > > > > + /* As we use 0 to signal that the Id field is not present, > > > > + * avoid confusion with packets without such field > > > > + */ > > > > + key_icmp->id = ih->un.echo.id ? : 1; > > > > > > Its not obvious to me why the kernel should treat id-zero as a special > > > value if it is not special on the wire. > > > > > > Perhaps a caller who needs to know if the id is present can > > > check the ICMP type as this code does, say using a helper. > > > > > > > Hi, > > > > The problem is that the 0-0 Type-Code pair identifies the echo replies. > > So instead of adding a bool is_present value I hardcoded the info in > > the ID field making it always non null, at the expense of a possible > > collision, which is harmless. > > Sorry, I feel that I'm missing something here. > > My reading of the code above is that for the cased types above > (echo, echo reply, ...) the id is present. Otherwise it is not. > My idea would be to put a check for those types in a helper. >
Something like icmp_has_id(), I like it.
> I do agree that the override you have used is harmless enough > in the context of the only user of the id which appears in > the following patch of this series. > > > Some other things I noticed in this patch on a second pass: > > * I think you can remove the icmp field from struct flow_dissector_key_ports >
You mean flow_dissector_key_icmp maybe?
> * I think that adding icmp to struct flow_keys should be accompanied by > adding ICMP to flow_keys_dissector_symmetric_keys. But I think this is > not desirable outside of the bonding use-case and rather > the bonding driver should define its own structures that > includes the keys it needs - basically copies of struct flow_keys > and flow_keys_dissector_symmetric_keys with some modifications. >
Just flow_keys_dissector_symmetric_keys or flow_keys_dissector_keys too? Anyway, it seems that the bonding uses the flow_dissector only when using encap2+3 or encap3+4 hashing, which means decap some known tunnels (mpls and gre and pppoe I think). For the other modes it just uses iph_to_flow_copy_v{4,6}addrs() and skb_flow_get_ports(), so maybe we can avoid copying that structure.
> * Modifying flow_keys_have_l4 affects the behaviour of > skb_get_hash_flowi6() but there is not a corresponding update > to flow_keys_have_l4(). I didn't look at all the other call sites > but it strikes me that this is a) a wide-spread behavioural change > and b) is perhaps not required for the bond-use case.
Right, no need to alter flow_keys_have_l4() at all.
I'll send a v2 with those suggestions.
Thanks, -- Matteo Croce per aspera ad upstream
| |