Thanks for the clarification. With that, for the API patch: Reviewed-by: Bill Fischofer <[email protected]>
I'll send a v2 of my implementation patch to reflect these implementation changes and will update the documentation accordingly. On Wed, Jan 27, 2016 at 2:59 AM, Savolainen, Petri (Nokia - FI/Espoo) < [email protected]> wrote: > > > > > *From:* EXT Bill Fischofer [mailto:[email protected]] > *Sent:* Tuesday, January 26, 2016 8:17 PM > *To:* Savolainen, Petri (Nokia - FI/Espoo) > *Cc:* LNG ODP Mailman List > *Subject:* Re: [lng-odp] [API-NEXT PATCH] api: packet: added multicast > flags > > > > 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. > > > > All these bits are for destination address. It would be a protocol error > (L2/L3 error flag set) in all these cases if source address would be bcast > or mcast. > > > > > > 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. > > > > Eth_bcast is the broadcast address - in practice all ones. I’d expect that > at least some IEEE Ethernet standard would define that as the bcast address. > > > > > > So do we really need both eth_bcast and eth_mcast? > > > > Yes. Multicast Ethernet addresses are many and handled differently (e.g. > in IP stack) than the (single) bcast address (all ones). > > > > For IP things are a bit more complicated. > > > > Parser needs to find only those addresses that are fixed by the protocol > address format. Dynamically configured addresses (like subnet bcast) would > need a proper IP stack, ODP is just a (dummy) parser – not a stack. > > > > 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? > > > > IPv4 multicast addresses are defined by the leading address bits of 1110 > (ip_mcast == 1). Broadcast is 255.255.255.255 (ip_bcast == 1). > > > > 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? > > > > IPv4 multicast addresses are defined by the leading address bits of > 11111111 (ip_mcast == 1). There’s no bcast address (ip_bcast == 0 always). > > > > > > -Petri > > > > > > 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
