Before sending my reviewed-by I started working on implementing these (along with the tests). The flags themselves are straightforward, however the parse definitions are somewhat complicated and we may want to simplify things a bit to capture what applications are really looking for.
First, we only define a single bit for each of these and it's not clear whether this applies to the source or destination address of the packet. For the sets, I'm assuming we'd want these to refer to the destination addresses since it makes little sense for an application to modify a source address since the packet has already been received, however on receipt do applications want to know whether the packet originated from a broadcast or multicast address? What are the use cases we're looking to support, since that will say whether we need one or two bits for these designators. Ethernet multicast is indicated by bit 7 of the MAC address being 1. By convention, most use MAC addr FF:FF:FF:FF:FF:FF to indicate broadcast even though Ethernet really doesn't distinguish between these two. So do we really need both eth_bcast and eth_mcast? For IP things are a bit more complicated. For IPv4, multicast is basically a Class D IP address (addresses in the range 224.x.x.x through 239.x.x.x (leading hex nibble D). Broadcast addresses, however, are a function of the subnet being used and that's not unambiguous in the absence of a separately specified subnet mask. For example, for network 192.168.1.0/8 the broadcast address is 192.168.1.255, however for network 192.168.1.0/4 the broadcast address is 192.168.1.15 whereas this is a unicast address in the /8 network. What exactly do we expect the ip_bcast bit to mean with respect to the (unknown) subnet mask? For IPv6 we have a similar issue in that IPv6 doesn't recognize broadcast addresses, but instead uses multicast addresses, which have prefix ff00::/8 with address ff02::1 being the "all nodes" multicast address, which is sort of like broadcast. Exactly how do we wish to define ip_bcast and ip_mcast for IPv6? On Fri, Jan 22, 2016 at 3:12 AM, Savolainen, Petri (Nokia - FI/Espoo) < [email protected]> wrote: > > > > > *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]> wrote: > > > > > > *From:* EXT Bill Fischofer [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]> 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]> > --- > 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] > https://lists.linaro.org/mailman/listinfo/lng-odp > > > > >
_______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
