From: EXT Bill Fischofer [mailto:[email protected]] Sent: Friday, January 22, 2016 1:03 AM To: Savolainen, Petri (Nokia - FI/Espoo) Cc: LNG ODP Mailman List Subject: Re: [lng-odp] [API-NEXT PATCH] api: packet: added multicast flags
On Thu, Jan 21, 2016 at 9:30 AM, Savolainen, Petri (Nokia - FI/Espoo) <[email protected]<mailto:[email protected]>> wrote: From: EXT Bill Fischofer [mailto:[email protected]<mailto:[email protected]>] Sent: Thursday, January 21, 2016 4:43 PM To: Savolainen, Petri (Nokia - FI/Espoo) Cc: LNG ODP Mailman List Subject: Re: [lng-odp] [API-NEXT PATCH] api: packet: added multicast flags On Thu, Jan 21, 2016 at 3:39 AM, Petri Savolainen <[email protected]<mailto:[email protected]>> wrote: Added packet flags for Ethernet and IP broad- and multicast. For application, it's more effective to check a flag than all destionation address bits. Signed-off-by: Petri Savolainen <[email protected]<mailto:[email protected]>> --- include/odp/api/packet_flags.h | 70 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 69 insertions(+), 1 deletion(-) diff --git a/include/odp/api/packet_flags.h b/include/odp/api/packet_flags.h index 8de0c82..65fd3ce 100644 --- a/include/odp/api/packet_flags.h +++ b/include/odp/api/packet_flags.h @@ -102,11 +102,29 @@ int odp_packet_has_l4(odp_packet_t pkt); * * @param pkt Packet handle * @retval non-zero if packet contains a valid eth header - * @retval 0 if packet does not contain a valid & known eth header + * @retval 0 if packet does not contain a valid eth header */ int odp_packet_has_eth(odp_packet_t pkt); /** + * Check for Ethernet broadcast address + * + * @param pkt Packet handle + * @retval non-zero if Ethernet destination address is the broadcast address + * @retval 0 if Ethernet destination address is not the broadcast address + */ +int odp_packet_has_eth_bcast(odp_packet_t pkt); + +/** + * Check for Ethernet multicast address + * + * @param pkt Packet handle + * @retval non-zero if Ethernet destination address is a multicast address + * @retval 0 if Ethernet destination address is not a multicast address + */ +int odp_packet_has_eth_mcast(odp_packet_t pkt); + +/** * Check for jumbo frame * * @param pkt Packet handle @@ -161,6 +179,24 @@ int odp_packet_has_ipv4(odp_packet_t pkt); int odp_packet_has_ipv6(odp_packet_t pkt); /** + * Check for IP broadcast address + * + * @param pkt Packet handle + * @retval non-zero if IP destination address is a broadcast address + * @retval 0 if IP destination address is not a broadcast address + */ +int odp_packet_has_ip_bcast(odp_packet_t pkt); + +/** + * Check for IP multicast address + * + * @param pkt Packet handle + * @retval non-zero if IP destination address is a multicast address + * @retval 0 if IP destination address is not a multicast address + */ +int odp_packet_has_ip_mcast(odp_packet_t pkt); + Should there be an odp_packet_has_ip_anycast() API for completeness? I don’t remember seeing that commonly supported by HW parsers whereas the two above are. It can be always added later if commonly needed and supported. Anycast is a mandatory part of IPv6 so I don't see why we'd want to omit it. Given the complexities of these parses, I'd imagine SW parsers would treat these as a separately-handled lazy parse case (i.e., no parse is done until one of these is actually called since the expectation would be that they'd be called rarely). There are many protocols with many mandatory features, but ODP is not a protocol stack. ODP should include only those protocol field checks that are commonly available in HW (and commonly needed by applications using ODP). We should avoid adding features for completeness. If most HW parsers output mcast and bcast flags, but none of them output an anycast flag - ODP should include mcast and bcast but not anycast. Could HW even distinguish anycast addresses from unicast/multicast addresses? HW depends on fixed address formats (like N most significant bits being 1), it would not (necessarily) know about host or router protocol stack settings. +/** * Check for IP fragment * * @param pkt Packet handle @@ -265,6 +301,22 @@ void odp_packet_has_l4_set(odp_packet_t pkt, int val); void odp_packet_has_eth_set(odp_packet_t pkt, int val); /** + * Set flag for Ethernet broadcast address + * + * @param pkt Packet handle + * @param val Value + */ +void odp_packet_has_eth_bcast_set(odp_packet_t pkt, int val); + +/** + * Set flag for Ethernet multicast address + * + * @param pkt Packet handle + * @param val Value + */ +void odp_packet_has_eth_mcast_set(odp_packet_t pkt, int val); + +/** * Set flag for jumbo frame * * @param pkt Packet handle @@ -313,6 +365,22 @@ void odp_packet_has_ipv4_set(odp_packet_t pkt, int val); void odp_packet_has_ipv6_set(odp_packet_t pkt, int val); /** + * Set flag for IP broadcast address + * + * @param pkt Packet handle + * @param val Value + */ +void odp_packet_has_ip_bcast_set(odp_packet_t pkt, int val); + +/** + * Set flag for IP multicast address + * + * @param pkt Packet handle + * @param val Value + */ +void odp_packet_has_ip_mcast_set(odp_packet_t pkt, int val); + The query APIs for these attributes are fine, however given that these are encoded in the Ethernet/IP addresses I'm not sure that set variants make sense here. Wouldn't these be indirectly changing the Ethernet/IP address? Especially in the case of IPv6 these tend to follow complex rules that aren't simple booleans. As any other flags, if user changes the packet (e.g. re-uses it for an unicast packet) or allocates a new one, he can reset / set relevant flags. The next module the pipeline (HW or SW) needs to see updated flags. -Petri As we add more of these attributes I continue to believe allowing the packet to be modified and the parse flags to be set independently is both wasteful (user needs to make multiple calls) and error-prone. The more such "set" calls there are the more likely it is that applications will forget to make one or do so in an inconsistent manner, at which point they lose their usefulness (and trustworthiness). It would be better to have an explicit odp_packet_parse() or odp_packet_parse_reset() API that would either force a reparse of the modified packet (resulting in a guaranteed known good parse) or simply reset the parse by indicating that the previous parse is now stale and resulting in a new lazy parse as needed should these attributes be subsequently referenced. Application needs to set/update only those flags that are needed in the next steps of the pipeline (next application or ODP blocks). When application modifies a packet it must keep track what it has changed. It would be a sign of bad application design if it would lose track on changes and run packet through a parser (again) to see what kind of packet it currently has. Also most HW accelerators parse packets once during packet input and you cannot use HW parser without circulating the packet back to the input (e.g. loopback interface). I have also thought to add a bit field struct of all packet flags that could be used (to read / write) multiple flags with a single call (instead of many individual calls). Also a “zero all flags” call could be added. Individual calls are still useful and provides best performance when user needs to check or set only couple of flags (commonly a SW layer is interested on a sub-set of flags). But these would be a topic for another patch. -Petri +/** * Set flag for IP fragment * * @param pkt Packet handle -- 2.6.3 _______________________________________________ lng-odp mailing list [email protected]<mailto:[email protected]> https://lists.linaro.org/mailman/listinfo/lng-odp
_______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
