I'll post a (crude) lazy eval patch to enable initial evaluation. Right now linux-generic and linux-dpdk have drifted apart in this area but I'm also working to re-converge things a bit to help keep non-DPDK-related maintenance in sync.
On Wed, Apr 29, 2015 at 10:41 AM, Ola Liljedahl <[email protected]> wrote: > On 29 April 2015 at 16:10, Bill Fischofer <[email protected]> > 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) < >> [email protected]> wrote: >> >>> > -----Original Message----- >>> > From: ext Bala Manoharan [mailto:[email protected]] >>> > Sent: Wednesday, April 29, 2015 11:12 AM >>> > To: Savolainen, Petri (Nokia - FI/Espoo) >>> > Cc: ext Zoltan Kiss; [email protected] >>> > 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) >>> > <[email protected]> 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:[email protected]] >>> > >> Sent: Wednesday, April 29, 2015 10:25 AM >>> > >> To: Savolainen, Petri (Nokia - FI/Espoo) >>> > >> Cc: ext Zoltan Kiss; [email protected] >>> > >> 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 < >>> [email protected]> >>> > >> 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) >>> > >> > <[email protected]> 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:[email protected]] >>> > >> >>> Sent: Tuesday, April 28, 2015 9:17 PM >>> > >> >>> To: Savolainen, Petri (Nokia - FI/Espoo); >>> [email protected] >>> > >> >>> 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:[email protected]] >>> > >> >>> >> Sent: Monday, April 27, 2015 8:29 PM >>> > >> >>> >> To: Savolainen, Petri (Nokia - FI/Espoo); lng- >>> > [email protected] >>> > >> >>> >> 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 <[email protected] >>> > >>> > >> >>> >>> --- >>> > >> >>> >>> 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 >>> > >> >> [email protected] >>> > >> >> https://lists.linaro.org/mailman/listinfo/lng-odp >>> _______________________________________________ >>> 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 >> >> >
_______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
