On 29 April 2015 at 16:10, Bill Fischofer <bill.fischo...@linaro.org> wrote:
> We need to be careful about trying to solve problems that haven't arisen > yet. There are several interrelated issues here: > > 1. Parsing (and classification) in SW takes cycles. This should only be > done once, not duplicated between the app and the ODP implementation, and > ideally should only be done to the degree that the results are actually > needed by the application. > > 2. The ultimate goal is for applications to make use of ODP's > parsing/classification capabilities since these will be HW-accelerated on > many platforms today, and on all platforms in the future. > > Therefore, any proposed changes to the API need to keep these two goals in > mind. The goal is to help transition apps away from doing > parsing/classification themselves, not to give increasingly complex (and > increasingly irrelevant in a HW-accelerated world) advice to the > implementation as to what they need. > > This is one of the reasons why none/all is a good starting point, since it > solves our immediate need for legacy apps running on SW implementations. > Those platforms that have zero-cost HW parsing can safely ignore a 'none' > spec with no ill effect, while SW platforms will eliminate unwanted > overhead. Beyond that, I believe simplest and most future-proof means of > addressing the above goals is to explore lazy evaluation approaches to SW > parsing in ODP. This involves no new APIs that would have only transitory > utility and is transparent to the application. > So we have a proposed solution which does not require any changes to the API. Why don't we try out this solution and see where it gets us? Changing the API (and making it more complicated, both to implement and to use) without having verified that this is the only solution is not a smart move. Keeping the architecture simple is an important goal (it will grow anyway but the role of an architect is to resist changes and propose alternative solutions using the existing mechanisms and API's). -- Ola > > > > On Wed, Apr 29, 2015 at 3:17 AM, Savolainen, Petri (Nokia - FI/Espoo) < > petri.savolai...@nokia.com> wrote: > >> > -----Original Message----- >> > From: ext Bala Manoharan [mailto:bala.manoha...@linaro.org] >> > Sent: Wednesday, April 29, 2015 11:12 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 >> > >> > 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? >> > >> >> The flags parameter has N different flags set. This function returns >> non-zero, if all "L2" flags are set. Other flags are ignored. >> >> > > >> > > // Set all L2 flags >> > > void odp_packet_parse_all_l2_set(odp_packet_parse_flags_t *flags); >> >> This sets all "L2" flags and does not touch other flags. >> >> -Petri >> >> >> > > >> > > ... >> > > >> > > >> > > 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- >> > o...@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 >> > > > _______________________________________________ > 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
