| Message ID | 20171205143514.4441-7-sven.eckelmann@openmesh.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | flow_dissector: Provide basic batman-adv unicast handling | expand |
On Tue, Dec 5, 2017 at 6:35 AM, Sven Eckelmann <sven.eckelmann@openmesh.com> wrote: > The batman-adv unicast packets contain a full layer 2 frame in encapsulated > form. The flow dissector must therefore be able to parse the batman-adv > unicast header to reach the layer 2+3 information. > > +--------------------+ > | ip(v6)hdr | > +--------------------+ > | inner ethhdr | > +--------------------+ > | batadv unicast hdr | > +--------------------+ > | outer ethhdr | > +--------------------+ > > The obtained information from the upper layer can then be used by RPS to > schedule the processing on separate cores. This allows better distribution > of multiple flows from the same neighbor to different cores. > > Signed-off-by: Sven Eckelmann <sven.eckelmann@openmesh.com> > --- > net/core/flow_dissector.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c > index 15ce30063765..784cc07fc58e 100644 > --- a/net/core/flow_dissector.c > +++ b/net/core/flow_dissector.c > @@ -24,6 +24,7 @@ > #include <linux/tcp.h> > #include <net/flow_dissector.h> > #include <scsi/fc/fc_fcoe.h> > +#include <uapi/linux/batadv.h> > > static void dissector_set_key(struct flow_dissector *flow_dissector, > enum flow_dissector_key_id key_id) > @@ -696,6 +697,35 @@ bool __skb_flow_dissect(const struct sk_buff *skb, > > break; > } > + case htons(ETH_P_BATMAN): { > + struct { > + struct batadv_unicast_packet batadv_unicast; > + struct ethhdr eth; > + } *hdr, _hdr; > + > + hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen, > + &_hdr); > + if (!hdr) { > + fdret = FLOW_DISSECT_RET_OUT_BAD; > + break; > + } > + > + if (hdr->batadv_unicast.version != BATADV_COMPAT_VERSION) { > + fdret = FLOW_DISSECT_RET_OUT_BAD; > + break; > + } > + > + if (hdr->batadv_unicast.packet_type != BATADV_UNICAST) { > + fdret = FLOW_DISSECT_RET_OUT_BAD; > + break; > + } > + > + proto = hdr->eth.h_proto; > + nhoff += sizeof(*hdr); > + > + fdret = FLOW_DISSECT_RET_PROTO_AGAIN; > + break; > + } > case htons(ETH_P_8021AD): > case htons(ETH_P_8021Q): { > const struct vlan_hdr *vlan; > -- > 2.11.0 > Switch statements with cases having many LOC is hard to read and __skb_flow_dissect is aleady quite convoluted to begin with. I suggest putting this in a static function similar to how MPLS and GRE are handled. Tom
On Dienstag, 5. Dezember 2017 09:19:45 CET Tom Herbert wrote: [...] > Switch statements with cases having many LOC is hard to read and > __skb_flow_dissect is aleady quite convoluted to begin with. > > I suggest putting this in a static function similar to how MPLS and > GRE are handled. Thanks for the feedback. I was not sure whether "inline" or an extra function would be preferred. I've then decided to implement it like most of the other protocols. But since an extra function is the preferred method for handling new protos, I will move it to an extra function. The change can already be found in https://git.open-mesh.org/linux-merge.git/shortlog/refs/heads/ecsv/flowdissector I also saw that you've introduced in commit 823b96939578 ("flow_dissector: Add control/reporting of encapsulation") a flag to stop dissecting when something encapsulated was detected. It is not 100% clear to me when the FLOW_DIS_ENCAPSULATION should be set and FLOW_DISSECTOR_F_STOP_AT_ENCAP be checked. Only when there is IP/eth in IP (like in the two examples from the commit message) or also when there is a ethernet header, followed by batman-adv unicast header and again followed by an ethernet header? Right now, the flag FLOW_DISSECTOR_F_STOP_AT_ENCAP is only used by fib_multipath_hash - so it doesn't really help me to understand it. But the FLOW_DIS_ENCAPSULATION is checked by drivers/net/ethernet/broadcom/bnxt/bnxt.c to set it in a special tunnel_type (anytunnel) mode for IPv4/IPv6 packets. So my (wild) guess right now is that these flags should only be used/checked when handling encapsulation inside IPv4/IPv6 packets. But maybe you can correct me here. Kind regards, Sven
On Wed, Dec 6, 2017 at 5:26 AM, Sven Eckelmann <sven.eckelmann@openmesh.com> wrote: > On Dienstag, 5. Dezember 2017 09:19:45 CET Tom Herbert wrote: > [...] >> Switch statements with cases having many LOC is hard to read and >> __skb_flow_dissect is aleady quite convoluted to begin with. >> >> I suggest putting this in a static function similar to how MPLS and >> GRE are handled. Perhaps it can even be behind a static key depending on whether any devices are active, adjusted in batadv_hardif_(en|dis)able_interface. > Thanks for the feedback. > > I was not sure whether "inline" or an extra function would be preferred. I've > then decided to implement it like most of the other protocols. But since an > extra function is the preferred method for handling new protos, I will move it > to an extra function. > > The change can already be found in > https://git.open-mesh.org/linux-merge.git/shortlog/refs/heads/ecsv/flowdissector > > I also saw that you've introduced in > commit 823b96939578 ("flow_dissector: Add control/reporting of encapsulation") > a flag to stop dissecting when something encapsulated was detected. It is not > 100% clear to me when the FLOW_DIS_ENCAPSULATION should be set and > FLOW_DISSECTOR_F_STOP_AT_ENCAP be checked. Only when there is IP/eth in IP > (like in the two examples from the commit message) or also when there is a > ethernet header, followed by batman-adv unicast header and again followed by > an ethernet header? Please implement FLOW_DISSECTOR_F_STOP_AT_ENCAP. It may be used in more flow dissector paths in the future. The features are also used by GRE, which can encap Ethernet, for an example that is closer to this protocol.
On Wed, Dec 6, 2017 at 8:54 AM, Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > On Wed, Dec 6, 2017 at 5:26 AM, Sven Eckelmann > <sven.eckelmann@openmesh.com> wrote: >> On Dienstag, 5. Dezember 2017 09:19:45 CET Tom Herbert wrote: >> [...] >>> Switch statements with cases having many LOC is hard to read and >>> __skb_flow_dissect is aleady quite convoluted to begin with. >>> >>> I suggest putting this in a static function similar to how MPLS and >>> GRE are handled. > > Perhaps it can even be behind a static key depending on whether any > devices are active, adjusted in batadv_hardif_(en|dis)able_interface. > It's aready in a switch statement so static key shouldn't make a difference. Also, we have made flow dissector operate indendently of whether to end protocol is enable (for instance GRE is dissected regarless of whether and GRE tunnels are configured). Tom >> Thanks for the feedback. >> >> I was not sure whether "inline" or an extra function would be preferred. I've >> then decided to implement it like most of the other protocols. But since an >> extra function is the preferred method for handling new protos, I will move it >> to an extra function. >> >> The change can already be found in >> https://git.open-mesh.org/linux-merge.git/shortlog/refs/heads/ecsv/flowdissector >> >> I also saw that you've introduced in >> commit 823b96939578 ("flow_dissector: Add control/reporting of encapsulation") >> a flag to stop dissecting when something encapsulated was detected. It is not >> 100% clear to me when the FLOW_DIS_ENCAPSULATION should be set and >> FLOW_DISSECTOR_F_STOP_AT_ENCAP be checked. Only when there is IP/eth in IP >> (like in the two examples from the commit message) or also when there is a >> ethernet header, followed by batman-adv unicast header and again followed by >> an ethernet header? > > Please implement FLOW_DISSECTOR_F_STOP_AT_ENCAP. It may > be used in more flow dissector paths in the future. > > The features are also used by GRE, which can encap Ethernet, for an example > that is closer to this protocol.
On Mittwoch, 6. Dezember 2017 11:54:13 CET Willem de Bruijn wrote: > Perhaps it can even be behind a static key depending on whether any > devices are active, adjusted in batadv_hardif_(en|dis)able_interface. I don't like that because we don't need batman-adv loaded to simply forward (bridge) traffic between interfaces. And not being able to use RPS with multiple cores just because the batman-adv module (and interfaces) is not enabled seems to be counter-intuitive. > Please implement FLOW_DISSECTOR_F_STOP_AT_ENCAP. It may > be used in more flow dissector paths in the future. > > The features are also used by GRE, which can encap Ethernet, for an example > that is closer to this protocol. Thanks for the feedback. I have it now implemented like in GRE. The change can already be found in https://git.open-mesh.org/linux-merge.git/shortlog/refs/heads/ecsv/flowdissector Kind regards, Sven
> On Mittwoch, 6. Dezember 2017 11:54:13 CET Willem de Bruijn wrote: >> Perhaps it can even be behind a static key depending on whether any >> devices are active, adjusted in batadv_hardif_(en|dis)able_interface. > > I don't like that because we don't need batman-adv loaded to simply forward > (bridge) traffic between interfaces. And not being able to use RPS with > multiple cores just because the batman-adv module (and interfaces) is not > enabled seems to be counter-intuitive. Okay. Ack on Tom's points, too. I want to be able to administratively limit the footprint of flow dissector, but agreed that this can be configured independent from the rest of the datapath, i.e., with knobs specific to flow dissector. >> Please implement FLOW_DISSECTOR_F_STOP_AT_ENCAP. It may >> be used in more flow dissector paths in the future. >> >> The features are also used by GRE, which can encap Ethernet, for an example >> that is closer to this protocol. > > Thanks for the feedback. I have it now implemented like in GRE. > > The change can already be found in > https://git.open-mesh.org/linux-merge.git/shortlog/refs/heads/ecsv/flowdissector Great, thanks.
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c index 15ce30063765..784cc07fc58e 100644 --- a/net/core/flow_dissector.c +++ b/net/core/flow_dissector.c @@ -24,6 +24,7 @@ #include <linux/tcp.h> #include <net/flow_dissector.h> #include <scsi/fc/fc_fcoe.h> +#include <uapi/linux/batadv.h> static void dissector_set_key(struct flow_dissector *flow_dissector, enum flow_dissector_key_id key_id) @@ -696,6 +697,35 @@ bool __skb_flow_dissect(const struct sk_buff *skb, break; } + case htons(ETH_P_BATMAN): { + struct { + struct batadv_unicast_packet batadv_unicast; + struct ethhdr eth; + } *hdr, _hdr; + + hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen, + &_hdr); + if (!hdr) { + fdret = FLOW_DISSECT_RET_OUT_BAD; + break; + } + + if (hdr->batadv_unicast.version != BATADV_COMPAT_VERSION) { + fdret = FLOW_DISSECT_RET_OUT_BAD; + break; + } + + if (hdr->batadv_unicast.packet_type != BATADV_UNICAST) { + fdret = FLOW_DISSECT_RET_OUT_BAD; + break; + } + + proto = hdr->eth.h_proto; + nhoff += sizeof(*hdr); + + fdret = FLOW_DISSECT_RET_PROTO_AGAIN; + break; + } case htons(ETH_P_8021AD): case htons(ETH_P_8021Q): { const struct vlan_hdr *vlan;
The batman-adv unicast packets contain a full layer 2 frame in encapsulated form. The flow dissector must therefore be able to parse the batman-adv unicast header to reach the layer 2+3 information. +--------------------+ | ip(v6)hdr | +--------------------+ | inner ethhdr | +--------------------+ | batadv unicast hdr | +--------------------+ | outer ethhdr | +--------------------+ The obtained information from the upper layer can then be used by RPS to schedule the processing on separate cores. This allows better distribution of multiple flows from the same neighbor to different cores. Signed-off-by: Sven Eckelmann <sven.eckelmann@openmesh.com> --- net/core/flow_dissector.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)