On 29 April 2015 at 13:24, Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com> wrote: > Hi, > > Possibly. Although, grouping adds struct complexity, so there should be some > benefit on doing that. I was going to add union "all" there, but decided to > leave it out since unnamed structs/unions are not defined in C99, so the both > the union and the bit field struct inside would have to be named (for strict > C99 compatibility). I think API needs to be strictly C99, other code > (linux-generic and tests) can relax a bit from that requirement. > > Instead of unions, I'd defined a set of (inline) functions: > > // Check if all L2 flags are set > // @return non-zero when all L2 level flags are set > int odp_packet_parse_all_l2(const odp_packet_parse_flags_t *flags);
One minor query on the above function, So when application calls the above function what will be the value of "flags"? will it have only l2 flags set or will have other layer flags also set? Regards, Bala > > // Set all L2 flags > void odp_packet_parse_all_l2_set(odp_packet_parse_flags_t *flags); > > ... > > > Also I think the implementation would not only check levels, but for specific > flags - especially those ones it does not have HW support. E.g. an > implementation may have everything else filled in by HW, but "sctp" or "tcp > options" flags. If the application does not ask for those, the implementation > is good to go as-is. If the application asks those, the implementation has to > choose a strategy to fill in those one way or another. > > > -Petri > >> -----Original Message----- >> From: ext Bala Manoharan [mailto:bala.manoha...@linaro.org] >> Sent: Wednesday, April 29, 2015 10:25 AM >> To: Savolainen, Petri (Nokia - FI/Espoo) >> Cc: ext Zoltan Kiss; lng-odp@lists.linaro.org >> Subject: Re: [lng-odp] [PATCH API-NEXT 4/5] api: packet_io: added parse >> mode >> >> Hi, >> >> Sorry I clicked send way too early in the previous mail :) >> >> We can optimize the odp_packet_parse_flags_t struct to support >> both layered parsing and individual parsing by the following way. >> >> I believe this might be useful for both application and implementation. >> >> typedef struct odp_packet_parse_flags_t { >> union { >> uint8_t l2_all; >> struct { >> uint8_t eth:1; /**< See odp_packet_has_eth() */ >> uint8_t jumbo:1; /**< See odp_packet_has_jumbo() */ >> uint8_t vlan:1; /**< See odp_packet_has_vlan() */ >> uint8_t vlan_qinq:1; /**< See >> odp_packet_has_vlan_qinq() */ >> }; >> }; >> union { >> uint8_t l2_all; >> struct { >> uint8_t arp:1; /**< See odp_packet_has_arp() */ >> uint8_t ipv4:1; /**< See odp_packet_has_ipv4() */ >> uint8_t ipv6:1; /**< See odp_packet_has_ipv6() */ >> uint8_t ipfrag:1; /**< See odp_packet_has_ipfrag() */ >> uint8_t ipopt:1; /**< See odp_packet_has_ipopt() */ >> uint8_t ipsec:1; /**< See odp_packet_has_ipsec() */ >> }; >> }; >> union { >> uint8_t l4_all; >> struct { >> uint8_t udp:1; /**< See odp_packet_has_udp() */ >> uint8_t tcp:1; /**< See odp_packet_has_tcp() */ >> uint8_t sctp:1; /**< See odp_packet_has_sctp() */ >> uint8_t icmp:1; /**< See odp_packet_has_icmp() */ >> }; >> }; >> } odp_packet_parse_flags_t; >> >> Regards, >> Bala >> >> On 29 April 2015 at 12:46, Bala Manoharan <bala.manoha...@linaro.org> >> wrote: >> > Hi, >> > >> > We can optimize the odp_packet_parse_flags_t in the following way to >> > handle the layered approach for parsing >> > >> > +typedef struct odp_packet_parse_flags_t { >> > >> > + uint32_t eth:1; /**< See odp_packet_has_eth() */ >> > + uint32_t jumbo:1; /**< See odp_packet_has_jumbo() */ >> > + uint32_t vlan:1; /**< See odp_packet_has_vlan() */ >> > + uint32_t vlan_qinq:1; /**< See odp_packet_has_vlan_qinq() */ >> > + uint32_t arp:1; /**< See odp_packet_has_arp() */ >> > + uint32_t ipv4:1; /**< See odp_packet_has_ipv4() */ >> > + uint32_t ipv6:1; /**< See odp_packet_has_ipv6() */ >> > + uint32_t ipfrag:1; /**< See odp_packet_has_ipfrag() */ >> > + uint32_t ipopt:1; /**< See odp_packet_has_ipopt() */ >> > + uint32_t ipsec:1; /**< See odp_packet_has_ipsec() */ >> > + uint32_t udp:1; /**< See odp_packet_has_udp() */ >> > + uint32_t tcp:1; /**< See odp_packet_has_tcp() */ >> > + uint32_t sctp:1; /**< See odp_packet_has_sctp() */ >> > + uint32_t icmp:1; /**< See odp_packet_has_icmp() */ >> > + >> > + uint32_t _reserved1:18; /**< Reserved. Do not use. */ >> > +} odp_packet_parse_flags_t; >> > >> > On 29 April 2015 at 12:15, Savolainen, Petri (Nokia - FI/Espoo) >> > <petri.savolai...@nokia.com> wrote: >> >> It's (v2) on the list (since last Thu): >> >> >> >> [lng-odp] [API-NEXT PATCH v2 4/5] api: packet_io: added parse mode >> >> >> >> >> >> -Petri >> >> >> >> >> >>> -----Original Message----- >> >>> From: ext Zoltan Kiss [mailto:zoltan.k...@linaro.org] >> >>> Sent: Tuesday, April 28, 2015 9:17 PM >> >>> To: Savolainen, Petri (Nokia - FI/Espoo); lng-odp@lists.linaro.org >> >>> Subject: Re: [lng-odp] [PATCH API-NEXT 4/5] api: packet_io: added >> parse >> >>> mode >> >>> >> >>> >> >>> >> >>> On 28/04/15 08:09, Savolainen, Petri (Nokia - FI/Espoo) wrote: >> >>> > Hi Zoltan, >> >>> > >> >>> > You should implement the latest version of the patch, which has only >> >>> ALL/NONE defined. We can leave SELECTED for later. >> >>> Ok, but where is that version? I could only find this one. >> >>> >> >>> > >> >>> > Briefly about SELECTED. The idea is that the application lists all >> >>> odp_packet_has_xxx() calls that it will call during packet processing. >> >>> Implementation can use that information to optimize parser >> functionality, >> >>> if it can. So, application is not telling to implementation what to do >> or >> >>> how to do it, but what information application is expecting from >> packets. >> >>> If application lies (indicates that it will not call xxx, but still >> calls >> >>> it), results are undefined. >> >>> > >> >>> > -Petri >> >>> > >> >>> > >> >>> >> -----Original Message----- >> >>> >> From: ext Zoltan Kiss [mailto:zoltan.k...@linaro.org] >> >>> >> Sent: Monday, April 27, 2015 8:29 PM >> >>> >> To: Savolainen, Petri (Nokia - FI/Espoo); lng-odp@lists.linaro.org >> >>> >> Subject: Re: [lng-odp] [PATCH API-NEXT 4/5] api: packet_io: added >> parse >> >>> >> mode >> >>> >> >> >>> >> >> >>> >> >> >>> >> On 20/04/15 13:10, Petri Savolainen wrote: >> >>> >>> Application can indicate which packet parsing results it is >> >>> >>> interested in (all, none or selected). >> >>> >>> >> >>> >>> Signed-off-by: Petri Savolainen <petri.savolai...@nokia.com> >> >>> >>> --- >> >>> >>> include/odp/api/packet_flags.h | 26 ++++++++++++++++++++++++++ >> >>> >>> include/odp/api/packet_io.h | 19 +++++++++++++++++++ >> >>> >>> 2 files changed, 45 insertions(+) >> >>> >>> >> >>> >>> diff --git a/include/odp/api/packet_flags.h >> >>> >> b/include/odp/api/packet_flags.h >> >>> >>> index bfbcc94..9444fdc 100644 >> >>> >>> --- a/include/odp/api/packet_flags.h >> >>> >>> +++ b/include/odp/api/packet_flags.h >> >>> >>> @@ -26,6 +26,32 @@ extern "C" { >> >>> >>> * @{ >> >>> >>> */ >> >>> >>> >> >>> >>> + >> >>> >>> +/** >> >>> >>> + * Packet input parsing flags >> >>> >>> + * >> >>> >>> + * Each flag represents a parser output. See parser output >> functions >> >>> >> for >> >>> >>> + * details. >> >>> >> >> >>> >> Now that I implement this for linux-generic, I realized this is >> >>> >> ambiguous: does disabling a lower layer's parsing means that the >> parser >> >>> >> stops looking into upper layers? Even if their parsing is enabled? >> >>> >> E.g. if (jumbo == 0 && ipv4 == 1), we won't parse the ethernet >> header >> >>> if >> >>> >> it's a jumbo frame, that's fine. But do we continue to look into >> the >> >>> >> rest of the packet for the IPv4 header? >> >>> >> I would say no, but it should be mentioned here explicitly. >> >>> >> >> >>> >>> + */ >> >>> >>> +typedef struct odp_packet_parse_flags_t { >> >>> >>> + uint32_t eth:1; /**< See odp_packet_has_eth() */ >> >>> >>> + uint32_t jumbo:1; /**< See odp_packet_has_jumbo() */ >> >>> >>> + uint32_t vlan:1; /**< See odp_packet_has_vlan() */ >> >>> >>> + uint32_t vlan_qinq:1; /**< See odp_packet_has_vlan_qinq() */ >> >>> >>> + uint32_t arp:1; /**< See odp_packet_has_arp() */ >> >>> >>> + uint32_t ipv4:1; /**< See odp_packet_has_ipv4() */ >> >>> >>> + uint32_t ipv6:1; /**< See odp_packet_has_ipv6() */ >> >>> >>> + uint32_t ipfrag:1; /**< See odp_packet_has_ipfrag() */ >> >>> >>> + uint32_t ipopt:1; /**< See odp_packet_has_ipopt() */ >> >>> >>> + uint32_t ipsec:1; /**< See odp_packet_has_ipsec() */ >> >>> >>> + uint32_t udp:1; /**< See odp_packet_has_udp() */ >> >>> >>> + uint32_t tcp:1; /**< See odp_packet_has_tcp() */ >> >>> >>> + uint32_t sctp:1; /**< See odp_packet_has_sctp() */ >> >>> >>> + uint32_t icmp:1; /**< See odp_packet_has_icmp() */ >> >>> >>> + >> >>> >>> + uint32_t _reserved1:18; /**< Reserved. Do not use. */ >> >>> >>> +} odp_packet_parse_flags_t; >> >>> >>> + >> >>> >>> /** >> >>> >>> * Check for packet errors >> >>> >>> * >> >>> >>> diff --git a/include/odp/api/packet_io.h >> b/include/odp/api/packet_io.h >> >>> >>> index 77c207e..97f79ee 100644 >> >>> >>> --- a/include/odp/api/packet_io.h >> >>> >>> +++ b/include/odp/api/packet_io.h >> >>> >>> @@ -18,6 +18,8 @@ >> >>> >>> extern "C" { >> >>> >>> #endif >> >>> >>> >> >>> >>> +#include <odp/packet_flags.h> >> >>> >>> + >> >>> >>> /** @defgroup odp_packet_io ODP PACKET IO >> >>> >>> * Operations on a packet. >> >>> >>> * @{ >> >>> >>> @@ -58,6 +60,19 @@ enum odp_pktio_input_mode { >> >>> >>> }; >> >>> >>> >> >>> >>> /** >> >>> >>> + * Packet parsing mode >> >>> >>> + */ >> >>> >>> +enum odp_pktio_parse_mode { >> >>> >>> + /** Parse all protocols */ >> >>> >>> + ODP_PKTIN_PARSE_ALL = 0, >> >>> >>> + /** Parsing not needed */ >> >>> >>> + ODP_PKTIN_PARSE_NONE, >> >>> >>> + /** Parsing can be limited to the flags set in >> >>> >>> + odp_packet_parse_flags_t */ >> >>> >>> + ODP_PKTIN_PARSE_SELECTED >> >>> >>> +}; >> >>> >>> + >> >>> >>> +/** >> >>> >>> * Packet IO parameters >> >>> >>> * >> >>> >>> * In minimum, user must select the input mode. Use 0 for >> defaults. >> >>> >> Initialize >> >>> >>> @@ -66,6 +81,10 @@ enum odp_pktio_input_mode { >> >>> >>> typedef struct odp_pktio_param_t { >> >>> >>> /** Packet input mode */ >> >>> >>> enum odp_pktio_input_mode in_mode; >> >>> >>> + /** Packet parse mode */ >> >>> >>> + enum odp_pktio_parse_mode parse_mode; >> >>> >>> + /** Parse selection when parse_mode is ODP_PKTIN_PARSE_SELECTED >> */ >> >>> >>> + odp_packet_parse_flags_t parse; >> >>> >>> } odp_pktio_param_t; >> >>> >>> >> >>> >>> /** >> >>> >>> >> >> _______________________________________________ >> >> lng-odp mailing list >> >> lng-odp@lists.linaro.org >> >> https://lists.linaro.org/mailman/listinfo/lng-odp _______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp
