Following discussions today I've posted v3 of this patch as two separate patches as described in the update to Bug 1334:
For v3 this patch has been split into two patches to reflect the change; Patch http://patches.opendataplane.org/patch/1215/ only changes the packet test in a way that is fully backwards-compatible with before, so it can be merged into the next ODP maintenance release (e.g., v1.0.2) Patch http://patches.opendataplane.org/patch/1214/ is posted to API-NEXT since even though it only changes the doxygen (documenting the change that these routines now use non-zero rather than 1 for true), any change to files in include/odp/api needs to be rolled into the next ODP API release. On Thu, Mar 26, 2015 at 10:05 AM, Ola Liljedahl <[email protected]> wrote: > On 26 March 2015 at 15:02, Savolainen, Petri (Nokia - FI/Espoo) < > [email protected]> wrote: > >> I was referring to flags in a SoC packet descriptor, which may be >> stored into one or more u64 words. >> > Code could access the most significant half (32 bits) of such 64-bit words > using 32-bit loads so avoid any need to shift. > Anyway I don't think a shift operation will cause any measurable overhead, > it might be executed in parallel with other instructions. > > >> I’m also suggesting to use int. Just wanted check if SoC vendors agree >> that it performs OK. >> >> >> >> -Petri >> >> >> >> >> >> >> >> *From:* ext Bill Fischofer [mailto:[email protected]] >> *Sent:* Thursday, March 26, 2015 3:13 PM >> *To:* Savolainen, Petri (Nokia - FI/Espoo) >> *Cc:* ext Maxim Uvarov; [email protected] >> *Subject:* Re: [lng-odp] [API-NEXT PATCHv2 1/3] api: packet: use >> odp_bool_t for packet flag APIs >> >> >> >> We actually have far fewer than 32 flag bits defined (19 at present), so >> no need for uint64_t to store them. Most SoCs have fairly efficient >> bit-twiddling instructions anyway so I'd leave the optimal implementation >> questions to them. >> >> >> >> Sounds like we should just leave this as int (as it is today), and just >> update the doxygen and the test to check for non-zero instead of 1. >> >> >> >> >> >> On Thu, Mar 26, 2015 at 7:55 AM, Savolainen, Petri (Nokia - FI/Espoo) < >> [email protected]> wrote: >> >> It’s OK to me. Although, I was thinking that an implementation defined >> type can save e.g. cycle in a case like this >> >> >> >> - flags are stored in a uint64_t >> >> - IPv4 flag in upper 32 bits >> >> - implementation defines the type as uint64_t >> >> - only one mask operation is needed e.g. >> >> return pkt_hrd.flags & HAS_IPV4_MASK; >> >> >> >> vs >> >> >> >> - return type is int >> >> - a mask and a shift are needed e.g. >> >> return (int)((pkt_hrd.flags & HAS_IPV4_MASK) >> 32); >> >> >> >> Is this a significant penalty? Only flags in the upper 32 bits need to be >> shifted. >> >> >> >> >> >> -Petri >> >> >> >> >> >> *From:* ext Bill Fischofer [mailto:[email protected]] >> *Sent:* Thursday, March 26, 2015 2:13 PM >> *To:* Savolainen, Petri (Nokia - FI/Espoo) >> *Cc:* ext Maxim Uvarov; [email protected] >> >> >> *Subject:* Re: [lng-odp] [API-NEXT PATCHv2 1/3] api: packet: use >> odp_bool_t for packet flag APIs >> >> >> >> Is there any reason not to leave this as int then? >> >> >> >> On Thu, Mar 26, 2015 at 7:10 AM, Savolainen, Petri (Nokia - FI/Espoo) < >> [email protected]> wrote: >> >> As discussed in one of the calls, I think 0 vs. non-zero return value is >> OK, but odp_bool_t would be misleading type for that range (bool is 0 or >> 1). So, it would be better to have another type for true/false integer >> condition. >> >> -Petri >> >> >> > -----Original Message----- >> > From: ext Maxim Uvarov [mailto:[email protected]] >> > Sent: Thursday, March 26, 2015 11:39 AM >> > To: [email protected] >> > Cc: Petri Savolainen >> > Subject: Re: [lng-odp] [API-NEXT PATCHv2 1/3] api: packet: use >> odp_bool_t >> > for packet flag APIs >> > >> > Petri, please review. >> > >> > Maxim. >> > >> > On 03/20/15 21:14, Bill Fischofer wrote: >> > > This change addresses Bug >> https://bugs.linaro.org/show_bug.cgi?id=1334 >> > > >> > > Signed-off-by: Bill Fischofer <[email protected]> >> > > --- >> > > >> > > v2 adds doxygen fix to this patch. Petri: Please review this for API >> > > conformance. >> > > >> > > include/odp/api/packet_flags.h | 72 >> +++++++++++++++++++++------------- >> > -------- >> > > 1 file changed, 36 insertions(+), 36 deletions(-) >> > > >> > > diff --git a/include/odp/api/packet_flags.h >> > b/include/odp/api/packet_flags.h >> > > index b1e179e..ccfb747 100644 >> > > --- a/include/odp/api/packet_flags.h >> > > +++ b/include/odp/api/packet_flags.h >> > > @@ -32,163 +32,163 @@ extern "C" { >> > > * Checks all error flags at once. >> > > * >> > > * @param pkt Packet handle >> > > - * @retval 1 packet has errors >> > > + * @retval non-zero packet has errors >> > > * @retval 0 packet has no errors >> > > */ >> > > -int odp_packet_has_error(odp_packet_t pkt); >> > > +odp_bool_t odp_packet_has_error(odp_packet_t pkt); >> > > >> > > /** >> > > * Check for L2 header, e.g. ethernet >> > > * >> > > * @param pkt Packet handle >> > > - * @retval 1 if packet contains a valid & known L2 header >> > > + * @retval non-zero if packet contains a valid & known L2 header >> > > * @retval 0 if packet does not contain a valid & known L2 header >> > > */ >> > > -int odp_packet_has_l2(odp_packet_t pkt); >> > > +odp_bool_t odp_packet_has_l2(odp_packet_t pkt); >> > > >> > > /** >> > > * Check for L3 header, e.g. IPv4, IPv6 >> > > * >> > > * @param pkt Packet handle >> > > - * @retval 1 if packet contains a valid & known L3 header >> > > + * @retval non-zero if packet contains a valid & known L3 header >> > > * @retval 0 if packet does not contain a valid & known L3 header >> > > */ >> > > -int odp_packet_has_l3(odp_packet_t pkt); >> > > +odp_bool_t odp_packet_has_l3(odp_packet_t pkt); >> > > >> > > /** >> > > * Check for L4 header, e.g. UDP, TCP, SCTP (also ICMP) >> > > * >> > > * @param pkt Packet handle >> > > - * @retval 1 if packet contains a valid & known L4 header >> > > + * @retval non-zero if packet contains a valid & known L4 header >> > > * @retval 0 if packet does not contain a valid & known L4 header >> > > */ >> > > -int odp_packet_has_l4(odp_packet_t pkt); >> > > +odp_bool_t odp_packet_has_l4(odp_packet_t pkt); >> > > >> > > /** >> > > * Check for Ethernet header >> > > * >> > > * @param pkt Packet handle >> > > - * @retval 1 if packet contains a valid eth header >> > > + * @retval non-zero if packet contains a valid eth header >> > > * @retval 0 if packet does not contain a valid & known eth header >> > > */ >> > > -int odp_packet_has_eth(odp_packet_t pkt); >> > > +odp_bool_t odp_packet_has_eth(odp_packet_t pkt); >> > > >> > > /** >> > > * Check for jumbo frame >> > > * >> > > * @param pkt Packet handle >> > > - * @retval 1 if packet contains a jumbo frame >> > > + * @retval non-zero if packet contains a jumbo frame >> > > * @retval 0 if packet does not contain a jumbo frame >> > > */ >> > > -int odp_packet_has_jumbo(odp_packet_t pkt); >> > > +odp_bool_t odp_packet_has_jumbo(odp_packet_t pkt); >> > > >> > > /** >> > > * Check for VLAN >> > > * >> > > * @param pkt Packet handle >> > > - * @retval 1 if packet contains a VLAN header >> > > + * @retval non-zero if packet contains a VLAN header >> > > * @retval 0 if packet does not contain a VLAN header >> > > */ >> > > -int odp_packet_has_vlan(odp_packet_t pkt); >> > > +odp_bool_t odp_packet_has_vlan(odp_packet_t pkt); >> > > >> > > /** >> > > * Check for VLAN QinQ (stacked VLAN) >> > > * >> > > * @param pkt Packet handle >> > > - * @retval 1 if packet contains a VLAN QinQ header >> > > + * @retval non-zero if packet contains a VLAN QinQ header >> > > * @retval 0 if packet does not contain a VLAN QinQ header >> > > */ >> > > -int odp_packet_has_vlan_qinq(odp_packet_t pkt); >> > > +odp_bool_t odp_packet_has_vlan_qinq(odp_packet_t pkt); >> > > >> > > /** >> > > * Check for ARP >> > > * >> > > * @param pkt Packet handle >> > > - * @retval 1 if packet contains an ARP message >> > > + * @retval non-zero if packet contains an ARP message >> > > * @retval 0 if packet does not contain an ARP message >> > > */ >> > > -int odp_packet_has_arp(odp_packet_t pkt); >> > > +odp_bool_t odp_packet_has_arp(odp_packet_t pkt); >> > > >> > > /** >> > > * Check for IPv4 >> > > * >> > > * @param pkt Packet handle >> > > - * @retval 1 if packet contains an IPv4 header >> > > + * @retval non-zero if packet contains an IPv4 header >> > > * @retval 0 if packet does not contain an IPv4 header >> > > */ >> > > -int odp_packet_has_ipv4(odp_packet_t pkt); >> > > +odp_bool_t odp_packet_has_ipv4(odp_packet_t pkt); >> > > >> > > /** >> > > * Check for IPv6 >> > > * >> > > * @param pkt Packet handle >> > > - * @retval 1 if packet contains an IPv6 header >> > > + * @retval non-zero if packet contains an IPv6 header >> > > * @retval 0 if packet does not contain an IPv6 header >> > > */ >> > > -int odp_packet_has_ipv6(odp_packet_t pkt); >> > > +odp_bool_t odp_packet_has_ipv6(odp_packet_t pkt); >> > > >> > > /** >> > > * Check for IP fragment >> > > * >> > > * @param pkt Packet handle >> > > - * @retval 1 if packet is an IP fragment >> > > + * @retval non-zero if packet is an IP fragment >> > > * @retval 0 if packet is not an IP fragment >> > > */ >> > > -int odp_packet_has_ipfrag(odp_packet_t pkt); >> > > +odp_bool_t odp_packet_has_ipfrag(odp_packet_t pkt); >> > > >> > > /** >> > > * Check for IP options >> > > * >> > > * @param pkt Packet handle >> > > - * @retval 1 if packet contains IP options >> > > + * @retval non-zero if packet contains IP options >> > > * @retval 0 if packet does not contain IP options >> > > */ >> > > -int odp_packet_has_ipopt(odp_packet_t pkt); >> > > +odp_bool_t odp_packet_has_ipopt(odp_packet_t pkt); >> > > >> > > /** >> > > * Check for IPSec >> > > * >> > > * @param pkt Packet handle >> > > - * @retval 1 if packet requires IPSec processing >> > > + * @retval non-zero if packet requires IPSec processing >> > > * @retval 0 if packet does not require IPSec processing >> > > */ >> > > -int odp_packet_has_ipsec(odp_packet_t pkt); >> > > +odp_bool_t odp_packet_has_ipsec(odp_packet_t pkt); >> > > >> > > /** >> > > * Check for UDP >> > > * >> > > * @param pkt Packet handle >> > > - * @retval 1 if packet contains a UDP header >> > > + * @retval non-zero if packet contains a UDP header >> > > * @retval 0 if packet does not contain a UDP header >> > > */ >> > > -int odp_packet_has_udp(odp_packet_t pkt); >> > > +odp_bool_t odp_packet_has_udp(odp_packet_t pkt); >> > > >> > > /** >> > > * Check for TCP >> > > * >> > > * @param pkt Packet handle >> > > - * @retval 1 if packet contains a TCP header >> > > + * @retval non-zero if packet contains a TCP header >> > > * @retval 0 if packet does not contain a TCP header >> > > */ >> > > -int odp_packet_has_tcp(odp_packet_t pkt); >> > > +odp_bool_t odp_packet_has_tcp(odp_packet_t pkt); >> > > >> > > /** >> > > * Check for SCTP >> > > * >> > > * @param pkt Packet handle >> > > - * @retval 1 if packet contains a SCTP header >> > > + * @retval non-zero if packet contains a SCTP header >> > > * @retval 0 if packet does not contain a SCTP header >> > > */ >> > > -int odp_packet_has_sctp(odp_packet_t pkt); >> > > +odp_bool_t odp_packet_has_sctp(odp_packet_t pkt); >> > > >> > > /** >> > > * Check for ICMP >> > > * >> > > * @param pkt Packet handle >> > > - * @retval 1 if packet contains an ICMP header >> > > + * @retval non-zero if packet contains an ICMP header >> > > * @retval 0 if packet does not contain an ICMP header >> > > */ >> > > -int odp_packet_has_icmp(odp_packet_t pkt); >> > > +odp_bool_t odp_packet_has_icmp(odp_packet_t pkt); >> > > >> > > /** >> > > * Set flag for L2 header, e.g. ethernet >> >> >> _______________________________________________ >> lng-odp mailing list >> [email protected] >> http://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >> >> >> _______________________________________________ >> lng-odp mailing list >> [email protected] >> http://lists.linaro.org/mailman/listinfo/lng-odp >> >> >
_______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
