On 27 January 2016 at 09:59, Savolainen, Petri (Nokia - FI/Espoo)
<[email protected] <mailto:[email protected]>> wrote:
*From:*EXT Bill Fischofer [mailto:[email protected]
<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.
Agree. In my experience (e.g. in the IP stacks I have written), IP
multicast and broadcast is detected by the IP layer when looking up
the destination IP address in the FIB. What to do with a packet
depends on the matching route (what action it specifies, e.g. drop,
forward, terminate). Thus I am not even sure I would need support in
the parser to detected IP multicast and broadcast addresses. But
perhaps other designs find these parser outputs useful.
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
<http://192.168.1.0/8> the broadcast address is 192.168.1.255,
however for network 192.168.1.0/4 <http://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]
<mailto:[email protected]>> wrote:
*From:*EXT Bill Fischofer [mailto:[email protected]
<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] <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
