| Message ID | 1429113656-14404-1-git-send-email-zoltan.kiss@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 16 April 2015 at 14:33, Savolainen, Petri (Nokia - FI/Espoo) < petri.savolainen@nokia.com> wrote: > Hi, > > I think this should be typed (as bit field) and part of the > odp_pktio_param_t params that I introduced in patch "api: packet_io: added > odp_pktio_param_t". I could rework those patches and add it there. > > Something like this, > > typedef struct odp_pktio_input_flags_t { > struct { > uint64_t eth:1; > uint64_t jumbo:1; > uint64_t vlan:1; > ... > > uint64_t _reserved:27; > }; > } odp_pktio_input_flags_t; > Do we really need to be able to specify what parsing to do to this level of detail? An implementation cannot efficiently check these all of these flags, the code would be drowning in conditionals. Why not specify whether parsing should be done for (or stop at) for layer-2, layer-3 and layer-4 (or not at all)? The functions in packet_flags.h must be associated with one of these levels. > > > typedef struct odp_pktio_param_t { > /** Packet input mode */ > enum odp_pktio_input_mode in_mode; > /** Packet input parser flags */ > odp_pktio_input_flags_t flags; > } odp_pktio_param_t; > > > odp_pktio_t odp_pktio_open(const char *dev, odp_pool_t pool, > const odp_pktio_param_t *param); > > > -Petri > > > > -----Original Message----- > > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of ext > > Zoltan Kiss > > Sent: Wednesday, April 15, 2015 7:01 PM > > To: lng-odp@lists.linaro.org > > Subject: [lng-odp] [RFC API-NEXT PATCH] packet_io: add bitmap to specify > > what level of parsing needed > > > > odp_pktio_open() will have a 32 bit bitmask to specify what kind of > header > > parsing is required by the application. > > > > Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org> > > --- > > include/odp/api/packet_flags.h | 49 > > ++++++++++++++++++++++++++++++++++++++++++ > > include/odp/api/packet_io.h | 14 ++++++++---- > > 2 files changed, 59 insertions(+), 4 deletions(-) > > > > diff --git a/include/odp/api/packet_flags.h > > b/include/odp/api/packet_flags.h > > index b1e179e..467d4f1 100644 > > --- a/include/odp/api/packet_flags.h > > +++ b/include/odp/api/packet_flags.h > > @@ -327,6 +327,55 @@ void odp_packet_has_sctp_set(odp_packet_t pkt, int > > val); > > void odp_packet_has_icmp_set(odp_packet_t pkt, int val); > > > > /** > > + * Shift values for enum odp_packet_parse. Has to be updated together > > with > > + * odp_packet_parse_e type. The parameter passed to odp_pktio_open is a > > 32 bit > > + * mask. > > + */ > > +typedef enum odp_packet_parse_shift { > > + ODP_PARSE_SHIFT_ETH, > > + ODP_PARSE_SHIFT_JUMBO, > > + ODP_PARSE_SHIFT_VLAN, > > + ODP_PARSE_SHIFT_VLAN_QINQ, > > + ODP_PARSE_SHIFT_ARP, > > + ODP_PARSE_SHIFT_IPV4, > > + ODP_PARSE_SHIFT_IPV6, > > + ODP_PARSE_SHIFT_IPFRAG, > > + ODP_PARSE_SHIFT_IPOPT, > > + ODP_PARSE_SHIFT_IPSEC, > > + ODP_PARSE_SHIFT_UDP, > > + ODP_PARSE_SHIFT_TCP, > > + ODP_PARSE_SHIFT_SCTP, > > + ODP_PARSE_SHIFT_ICMP, > > + ODP_PARSE_SHIFT_MAX = 31 > > +} odp_packet_parse_shift_e; > > + > > +#define ODP_PARSE(FIELD) ODP_PARSE_##FIELD = (ODP_PARSE_SHIFT_##FIELD << > > 1) > > + > > +/** > > + * Values to be used when calling odp_pktio_open. The parser_mask > > parameter has > > + * to be one or more of these values joined with bitwise OR. Or one of > > the two > > + * special values: ODP_PARSE_NONE or ODP_PARSE_ALL. > > + * Has to be updated together with odp_packet_parse_shift_e > > + */ > > +typedef enum odp_packet_parse { > > + ODP_PARSE_NONE = 0, /* The application don't want any > parsing */ > > + ODP_PARSE(ETH), /* Parse Ethernet header */ > > + ODP_PARSE(JUMBO), /* Parse Ethernet header if a jumbo frame > */ > > + ODP_PARSE(VLAN), /* Parse VLAN header */ > > + ODP_PARSE(VLAN_QINQ), /* Parse VLAN QinQ header */ > > + ODP_PARSE(ARP), /* Parse ARP header */ > > + ODP_PARSE(IPV4), /* Parse IPv4 header */ > > + ODP_PARSE(IPV6), /* Parse IPv6 header */ > > + ODP_PARSE(IPFRAG), /* Parse IPv4 header if fragmented */ > > + ODP_PARSE(IPOPT), /* Parse IP options */ > > + ODP_PARSE(IPSEC), /* Parse IPsec header */ > > + ODP_PARSE(UDP), /* Parse UDP header */ > > + ODP_PARSE(TCP), /* Parse TCP header */ > > + ODP_PARSE(SCTP), /* Parse SCTP header */ > > + ODP_PARSE(ICMP), /* Parse ICMP header */ > > + ODP_PARSE_ALL = UINT32_MAX /* The application wants full parsing */ > > +} odp_packet_parse_e; > > +/** > > * @} > > */ > > > > diff --git a/include/odp/api/packet_io.h b/include/odp/api/packet_io.h > > index 6d31aeb..8c989f3 100644 > > --- a/include/odp/api/packet_io.h > > +++ b/include/odp/api/packet_io.h > > @@ -51,9 +51,14 @@ extern "C" { > > * open device will fail, returning ODP_PKTIO_INVALID with errno set. > > * odp_pktio_lookup() may be used to obtain a handle to an already open > > device. > > * > > - * @param dev Packet IO device name > > - * @param pool Pool from which to allocate buffers for storing packets > > - * received over this packet IO > > + * @param dev Packet IO device name > > + * @param pool Pool from which to allocate buffers for storing > > packets > > + * received over this packet IO > > + * @param parsing_mask Mask to request parsing. Must be one or more > > ODP_PARSE_* > > + * value joined with bitwise OR to request > > particular > > + * fields to be parsed. Or one of two special > > values: > > + * ODP_PARSE_NONE or ODP_PARSE_ALL. See > > odp_packet_parse_e > > + * in packet_flags.h for more details > > * > > * @return ODP packet IO handle > > * @retval ODP_PKTIO_INVALID on failure > > @@ -62,7 +67,8 @@ extern "C" { > > * device used for testing. Usually it's loop back > > * interface. > > */ > > -odp_pktio_t odp_pktio_open(const char *dev, odp_pool_t pool); > > +odp_pktio_t odp_pktio_open(const char *dev, odp_pool_t pool, > > + uint32_t parsing_mask); > > > > /** > > * Close an ODP packet IO instance > > -- > > 1.9.1 > > > > _______________________________________________ > > 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 >
On 16 April 2015 at 15:37, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > > On 16 April 2015 at 14:33, Savolainen, Petri (Nokia - FI/Espoo) < > petri.savolainen@nokia.com> wrote: > >> Hi, >> >> I think this should be typed (as bit field) and part of the >> odp_pktio_param_t params that I introduced in patch "api: packet_io: added >> odp_pktio_param_t". I could rework those patches and add it there. >> >> Something like this, >> >> typedef struct odp_pktio_input_flags_t { >> struct { >> uint64_t eth:1; >> uint64_t jumbo:1; >> uint64_t vlan:1; >> ... >> >> uint64_t _reserved:27; >> }; >> } odp_pktio_input_flags_t; >> > Do we really need to be able to specify what parsing to do to this level > of detail? An implementation cannot efficiently check these all of these > flags, the code would be drowning in conditionals. Why not specify whether > parsing should be done for (or stop at) for layer-2, layer-3 and layer-4 > (or not at all)? The functions in packet_flags.h must be associated with > one of these levels. > Of course a (SW) parser implementation could aggregate the specific bit flags and compute the corresponding parsing levels and use that when deciding how far into the packet to parse. > >> >> >> typedef struct odp_pktio_param_t { >> /** Packet input mode */ >> enum odp_pktio_input_mode in_mode; >> /** Packet input parser flags */ >> odp_pktio_input_flags_t flags; >> } odp_pktio_param_t; >> >> >> odp_pktio_t odp_pktio_open(const char *dev, odp_pool_t pool, >> const odp_pktio_param_t *param); >> >> >> -Petri >> >> >> > -----Original Message----- >> > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of >> ext >> > Zoltan Kiss >> > Sent: Wednesday, April 15, 2015 7:01 PM >> > To: lng-odp@lists.linaro.org >> > Subject: [lng-odp] [RFC API-NEXT PATCH] packet_io: add bitmap to specify >> > what level of parsing needed >> > >> > odp_pktio_open() will have a 32 bit bitmask to specify what kind of >> header >> > parsing is required by the application. >> > >> > Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org> >> > --- >> > include/odp/api/packet_flags.h | 49 >> > ++++++++++++++++++++++++++++++++++++++++++ >> > include/odp/api/packet_io.h | 14 ++++++++---- >> > 2 files changed, 59 insertions(+), 4 deletions(-) >> > >> > diff --git a/include/odp/api/packet_flags.h >> > b/include/odp/api/packet_flags.h >> > index b1e179e..467d4f1 100644 >> > --- a/include/odp/api/packet_flags.h >> > +++ b/include/odp/api/packet_flags.h >> > @@ -327,6 +327,55 @@ void odp_packet_has_sctp_set(odp_packet_t pkt, int >> > val); >> > void odp_packet_has_icmp_set(odp_packet_t pkt, int val); >> > >> > /** >> > + * Shift values for enum odp_packet_parse. Has to be updated together >> > with >> > + * odp_packet_parse_e type. The parameter passed to odp_pktio_open is a >> > 32 bit >> > + * mask. >> > + */ >> > +typedef enum odp_packet_parse_shift { >> > + ODP_PARSE_SHIFT_ETH, >> > + ODP_PARSE_SHIFT_JUMBO, >> > + ODP_PARSE_SHIFT_VLAN, >> > + ODP_PARSE_SHIFT_VLAN_QINQ, >> > + ODP_PARSE_SHIFT_ARP, >> > + ODP_PARSE_SHIFT_IPV4, >> > + ODP_PARSE_SHIFT_IPV6, >> > + ODP_PARSE_SHIFT_IPFRAG, >> > + ODP_PARSE_SHIFT_IPOPT, >> > + ODP_PARSE_SHIFT_IPSEC, >> > + ODP_PARSE_SHIFT_UDP, >> > + ODP_PARSE_SHIFT_TCP, >> > + ODP_PARSE_SHIFT_SCTP, >> > + ODP_PARSE_SHIFT_ICMP, >> > + ODP_PARSE_SHIFT_MAX = 31 >> > +} odp_packet_parse_shift_e; >> > + >> > +#define ODP_PARSE(FIELD) ODP_PARSE_##FIELD = (ODP_PARSE_SHIFT_##FIELD >> << >> > 1) >> > + >> > +/** >> > + * Values to be used when calling odp_pktio_open. The parser_mask >> > parameter has >> > + * to be one or more of these values joined with bitwise OR. Or one of >> > the two >> > + * special values: ODP_PARSE_NONE or ODP_PARSE_ALL. >> > + * Has to be updated together with odp_packet_parse_shift_e >> > + */ >> > +typedef enum odp_packet_parse { >> > + ODP_PARSE_NONE = 0, /* The application don't want any >> parsing */ >> > + ODP_PARSE(ETH), /* Parse Ethernet header */ >> > + ODP_PARSE(JUMBO), /* Parse Ethernet header if a jumbo frame >> */ >> > + ODP_PARSE(VLAN), /* Parse VLAN header */ >> > + ODP_PARSE(VLAN_QINQ), /* Parse VLAN QinQ header */ >> > + ODP_PARSE(ARP), /* Parse ARP header */ >> > + ODP_PARSE(IPV4), /* Parse IPv4 header */ >> > + ODP_PARSE(IPV6), /* Parse IPv6 header */ >> > + ODP_PARSE(IPFRAG), /* Parse IPv4 header if fragmented */ >> > + ODP_PARSE(IPOPT), /* Parse IP options */ >> > + ODP_PARSE(IPSEC), /* Parse IPsec header */ >> > + ODP_PARSE(UDP), /* Parse UDP header */ >> > + ODP_PARSE(TCP), /* Parse TCP header */ >> > + ODP_PARSE(SCTP), /* Parse SCTP header */ >> > + ODP_PARSE(ICMP), /* Parse ICMP header */ >> > + ODP_PARSE_ALL = UINT32_MAX /* The application wants full parsing */ >> > +} odp_packet_parse_e; >> > +/** >> > * @} >> > */ >> > >> > diff --git a/include/odp/api/packet_io.h b/include/odp/api/packet_io.h >> > index 6d31aeb..8c989f3 100644 >> > --- a/include/odp/api/packet_io.h >> > +++ b/include/odp/api/packet_io.h >> > @@ -51,9 +51,14 @@ extern "C" { >> > * open device will fail, returning ODP_PKTIO_INVALID with errno set. >> > * odp_pktio_lookup() may be used to obtain a handle to an already open >> > device. >> > * >> > - * @param dev Packet IO device name >> > - * @param pool Pool from which to allocate buffers for storing >> packets >> > - * received over this packet IO >> > + * @param dev Packet IO device name >> > + * @param pool Pool from which to allocate buffers for storing >> > packets >> > + * received over this packet IO >> > + * @param parsing_mask Mask to request parsing. Must be one or more >> > ODP_PARSE_* >> > + * value joined with bitwise OR to request >> > particular >> > + * fields to be parsed. Or one of two special >> > values: >> > + * ODP_PARSE_NONE or ODP_PARSE_ALL. See >> > odp_packet_parse_e >> > + * in packet_flags.h for more details >> > * >> > * @return ODP packet IO handle >> > * @retval ODP_PKTIO_INVALID on failure >> > @@ -62,7 +67,8 @@ extern "C" { >> > * device used for testing. Usually it's loop back >> > * interface. >> > */ >> > -odp_pktio_t odp_pktio_open(const char *dev, odp_pool_t pool); >> > +odp_pktio_t odp_pktio_open(const char *dev, odp_pool_t pool, >> > + uint32_t parsing_mask); >> > >> > /** >> > * Close an ODP packet IO instance >> > -- >> > 1.9.1 >> > >> > _______________________________________________ >> > 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 >> > >
On 16/04/15 13:33, Savolainen, Petri (Nokia - FI/Espoo) wrote: > Hi, > > I think this should be typed (as bit field) and part of the odp_pktio_param_t params that I introduced in patch "api: packet_io: added odp_pktio_param_t". I could rework those patches and add it there. Ok, that would be probably even better. > > Something like this, > > typedef struct odp_pktio_input_flags_t { > struct { > uint64_t eth:1; > uint64_t jumbo:1; > uint64_t vlan:1; > ... > > uint64_t _reserved:27; > }; > } odp_pktio_input_flags_t; I think it would be better to have a name which contains "parse" in some way. > > > typedef struct odp_pktio_param_t { > /** Packet input mode */ > enum odp_pktio_input_mode in_mode; > /** Packet input parser flags */ > odp_pktio_input_flags_t flags; > } odp_pktio_param_t; > > > odp_pktio_t odp_pktio_open(const char *dev, odp_pool_t pool, > const odp_pktio_param_t *param); > > > -Petri > > >> -----Original Message----- >> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of ext >> Zoltan Kiss >> Sent: Wednesday, April 15, 2015 7:01 PM >> To: lng-odp@lists.linaro.org >> Subject: [lng-odp] [RFC API-NEXT PATCH] packet_io: add bitmap to specify >> what level of parsing needed >> >> odp_pktio_open() will have a 32 bit bitmask to specify what kind of header >> parsing is required by the application. >> >> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org> >> --- >> include/odp/api/packet_flags.h | 49 >> ++++++++++++++++++++++++++++++++++++++++++ >> include/odp/api/packet_io.h | 14 ++++++++---- >> 2 files changed, 59 insertions(+), 4 deletions(-) >> >> diff --git a/include/odp/api/packet_flags.h >> b/include/odp/api/packet_flags.h >> index b1e179e..467d4f1 100644 >> --- a/include/odp/api/packet_flags.h >> +++ b/include/odp/api/packet_flags.h >> @@ -327,6 +327,55 @@ void odp_packet_has_sctp_set(odp_packet_t pkt, int >> val); >> void odp_packet_has_icmp_set(odp_packet_t pkt, int val); >> >> /** >> + * Shift values for enum odp_packet_parse. Has to be updated together >> with >> + * odp_packet_parse_e type. The parameter passed to odp_pktio_open is a >> 32 bit >> + * mask. >> + */ >> +typedef enum odp_packet_parse_shift { >> + ODP_PARSE_SHIFT_ETH, >> + ODP_PARSE_SHIFT_JUMBO, >> + ODP_PARSE_SHIFT_VLAN, >> + ODP_PARSE_SHIFT_VLAN_QINQ, >> + ODP_PARSE_SHIFT_ARP, >> + ODP_PARSE_SHIFT_IPV4, >> + ODP_PARSE_SHIFT_IPV6, >> + ODP_PARSE_SHIFT_IPFRAG, >> + ODP_PARSE_SHIFT_IPOPT, >> + ODP_PARSE_SHIFT_IPSEC, >> + ODP_PARSE_SHIFT_UDP, >> + ODP_PARSE_SHIFT_TCP, >> + ODP_PARSE_SHIFT_SCTP, >> + ODP_PARSE_SHIFT_ICMP, >> + ODP_PARSE_SHIFT_MAX = 31 >> +} odp_packet_parse_shift_e; >> + >> +#define ODP_PARSE(FIELD) ODP_PARSE_##FIELD = (ODP_PARSE_SHIFT_##FIELD << >> 1) >> + >> +/** >> + * Values to be used when calling odp_pktio_open. The parser_mask >> parameter has >> + * to be one or more of these values joined with bitwise OR. Or one of >> the two >> + * special values: ODP_PARSE_NONE or ODP_PARSE_ALL. >> + * Has to be updated together with odp_packet_parse_shift_e >> + */ >> +typedef enum odp_packet_parse { >> + ODP_PARSE_NONE = 0, /* The application don't want any parsing */ >> + ODP_PARSE(ETH), /* Parse Ethernet header */ >> + ODP_PARSE(JUMBO), /* Parse Ethernet header if a jumbo frame */ >> + ODP_PARSE(VLAN), /* Parse VLAN header */ >> + ODP_PARSE(VLAN_QINQ), /* Parse VLAN QinQ header */ >> + ODP_PARSE(ARP), /* Parse ARP header */ >> + ODP_PARSE(IPV4), /* Parse IPv4 header */ >> + ODP_PARSE(IPV6), /* Parse IPv6 header */ >> + ODP_PARSE(IPFRAG), /* Parse IPv4 header if fragmented */ >> + ODP_PARSE(IPOPT), /* Parse IP options */ >> + ODP_PARSE(IPSEC), /* Parse IPsec header */ >> + ODP_PARSE(UDP), /* Parse UDP header */ >> + ODP_PARSE(TCP), /* Parse TCP header */ >> + ODP_PARSE(SCTP), /* Parse SCTP header */ >> + ODP_PARSE(ICMP), /* Parse ICMP header */ >> + ODP_PARSE_ALL = UINT32_MAX /* The application wants full parsing */ >> +} odp_packet_parse_e; >> +/** >> * @} >> */ >> >> diff --git a/include/odp/api/packet_io.h b/include/odp/api/packet_io.h >> index 6d31aeb..8c989f3 100644 >> --- a/include/odp/api/packet_io.h >> +++ b/include/odp/api/packet_io.h >> @@ -51,9 +51,14 @@ extern "C" { >> * open device will fail, returning ODP_PKTIO_INVALID with errno set. >> * odp_pktio_lookup() may be used to obtain a handle to an already open >> device. >> * >> - * @param dev Packet IO device name >> - * @param pool Pool from which to allocate buffers for storing packets >> - * received over this packet IO >> + * @param dev Packet IO device name >> + * @param pool Pool from which to allocate buffers for storing >> packets >> + * received over this packet IO >> + * @param parsing_mask Mask to request parsing. Must be one or more >> ODP_PARSE_* >> + * value joined with bitwise OR to request >> particular >> + * fields to be parsed. Or one of two special >> values: >> + * ODP_PARSE_NONE or ODP_PARSE_ALL. See >> odp_packet_parse_e >> + * in packet_flags.h for more details >> * >> * @return ODP packet IO handle >> * @retval ODP_PKTIO_INVALID on failure >> @@ -62,7 +67,8 @@ extern "C" { >> * device used for testing. Usually it's loop back >> * interface. >> */ >> -odp_pktio_t odp_pktio_open(const char *dev, odp_pool_t pool); >> +odp_pktio_t odp_pktio_open(const char *dev, odp_pool_t pool, >> + uint32_t parsing_mask); >> >> /** >> * Close an ODP packet IO instance >> -- >> 1.9.1 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp
From: ext Ola Liljedahl [mailto:ola.liljedahl@linaro.org]
Sent: Thursday, April 16, 2015 5:03 PM
To: Savolainen, Petri (Nokia - FI/Espoo)
Cc: ext Zoltan Kiss; lng-odp@lists.linaro.org
Subject: Re: [lng-odp] [RFC API-NEXT PATCH] packet_io: add bitmap to specify what level of parsing needed
On 16 April 2015 at 15:37, Ola Liljedahl <ola.liljedahl@linaro.org<mailto:ola.liljedahl@linaro.org>> wrote:
Do we really need to be able to specify what parsing to do to this level of detail? An implementation cannot efficiently check these all of these flags, the code would be drowning in conditionals. Why not specify whether parsing should be done for (or stop at) for layer-2, layer-3 and layer-4 (or not at all)? The functions in packet_flags.h must be associated with one of these levels.
Of course a (SW) parser implementation could aggregate the specific bit flags and compute the corresponding parsing levels and use that when deciding how far into the packet to parse.
This tells to the implementation which parse results ( odp_packet_has_xxx() ) the application is going to use. If e.g. application will never call odp_packet_has_sctp() (or some more detailed, new flag we’d add in the future: odp_packet_has_stcp_xxx()) and the implementation does not have HW support for that, implementation could now skip SW parsing of that protocol altogether. It does not hurt (no SW overhead) if implementation has HW support for the protocol and does parsing in any case.
Results are undefined, if application first claims not to use a parse results, but then uses that after all (e.g. _has_xxx() may return 0, even when the packet has xxx).
-Petri
On 17/04/15 07:53, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > >> -----Original Message----- >> From: ext Zoltan Kiss [mailto:zoltan.kiss@linaro.org] >> Sent: Thursday, April 16, 2015 11:05 PM >> To: Savolainen, Petri (Nokia - FI/Espoo); lng-odp@lists.linaro.org >> Subject: Re: [lng-odp] [RFC API-NEXT PATCH] packet_io: add bitmap to >> specify what level of parsing needed >> >> >> >> On 16/04/15 13:33, Savolainen, Petri (Nokia - FI/Espoo) wrote: >>> Hi, >>> >>> I think this should be typed (as bit field) and part of the >> odp_pktio_param_t params that I introduced in patch "api: packet_io: added >> odp_pktio_param_t". I could rework those patches and add it there. >> Ok, that would be probably even better. >> >>> >>> Something like this, >>> >>> typedef struct odp_pktio_input_flags_t { >>> struct { >>> uint64_t eth:1; >>> uint64_t jumbo:1; >>> uint64_t vlan:1; >>> ... >>> >>> uint64_t _reserved:27; >>> }; >>> } odp_pktio_input_flags_t; >> I think it would be better to have a name which contains "parse" in some >> way. > > This same definition could be reused somewhere else in the API ... > >> >>> >>> >>> typedef struct odp_pktio_param_t { >>> /** Packet input mode */ >>> enum odp_pktio_input_mode in_mode; >>> /** Packet input parser flags */ >>> odp_pktio_input_flags_t flags; > > ... but here it could be contained. > > odp_pktio_input_flags_t parse; Ok, sounds good. > > -petri > >>> } odp_pktio_param_t; >>> >>> >>> odp_pktio_t odp_pktio_open(const char *dev, odp_pool_t pool, >>> const odp_pktio_param_t *param); >>> >>> >>> -Petri >>> >>> >>>> -----Original Message----- >>>> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of >> ext >>>> Zoltan Kiss >>>> Sent: Wednesday, April 15, 2015 7:01 PM >>>> To: lng-odp@lists.linaro.org >>>> Subject: [lng-odp] [RFC API-NEXT PATCH] packet_io: add bitmap to >> specify >>>> what level of parsing needed >>>> >>>> odp_pktio_open() will have a 32 bit bitmask to specify what kind of >> header >>>> parsing is required by the application. >>>> >>>> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org> >>>> --- >>>> include/odp/api/packet_flags.h | 49 >>>> ++++++++++++++++++++++++++++++++++++++++++ >>>> include/odp/api/packet_io.h | 14 ++++++++---- >>>> 2 files changed, 59 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/include/odp/api/packet_flags.h >>>> b/include/odp/api/packet_flags.h >>>> index b1e179e..467d4f1 100644 >>>> --- a/include/odp/api/packet_flags.h >>>> +++ b/include/odp/api/packet_flags.h >>>> @@ -327,6 +327,55 @@ void odp_packet_has_sctp_set(odp_packet_t pkt, int >>>> val); >>>> void odp_packet_has_icmp_set(odp_packet_t pkt, int val); >>>> >>>> /** >>>> + * Shift values for enum odp_packet_parse. Has to be updated together >>>> with >>>> + * odp_packet_parse_e type. The parameter passed to odp_pktio_open is >> a >>>> 32 bit >>>> + * mask. >>>> + */ >>>> +typedef enum odp_packet_parse_shift { >>>> + ODP_PARSE_SHIFT_ETH, >>>> + ODP_PARSE_SHIFT_JUMBO, >>>> + ODP_PARSE_SHIFT_VLAN, >>>> + ODP_PARSE_SHIFT_VLAN_QINQ, >>>> + ODP_PARSE_SHIFT_ARP, >>>> + ODP_PARSE_SHIFT_IPV4, >>>> + ODP_PARSE_SHIFT_IPV6, >>>> + ODP_PARSE_SHIFT_IPFRAG, >>>> + ODP_PARSE_SHIFT_IPOPT, >>>> + ODP_PARSE_SHIFT_IPSEC, >>>> + ODP_PARSE_SHIFT_UDP, >>>> + ODP_PARSE_SHIFT_TCP, >>>> + ODP_PARSE_SHIFT_SCTP, >>>> + ODP_PARSE_SHIFT_ICMP, >>>> + ODP_PARSE_SHIFT_MAX = 31 >>>> +} odp_packet_parse_shift_e; >>>> + >>>> +#define ODP_PARSE(FIELD) ODP_PARSE_##FIELD = (ODP_PARSE_SHIFT_##FIELD >> << >>>> 1) >>>> + >>>> +/** >>>> + * Values to be used when calling odp_pktio_open. The parser_mask >>>> parameter has >>>> + * to be one or more of these values joined with bitwise OR. Or one of >>>> the two >>>> + * special values: ODP_PARSE_NONE or ODP_PARSE_ALL. >>>> + * Has to be updated together with odp_packet_parse_shift_e >>>> + */ >>>> +typedef enum odp_packet_parse { >>>> + ODP_PARSE_NONE = 0, /* The application don't want any >> parsing */ >>>> + ODP_PARSE(ETH), /* Parse Ethernet header */ >>>> + ODP_PARSE(JUMBO), /* Parse Ethernet header if a jumbo frame */ >>>> + ODP_PARSE(VLAN), /* Parse VLAN header */ >>>> + ODP_PARSE(VLAN_QINQ), /* Parse VLAN QinQ header */ >>>> + ODP_PARSE(ARP), /* Parse ARP header */ >>>> + ODP_PARSE(IPV4), /* Parse IPv4 header */ >>>> + ODP_PARSE(IPV6), /* Parse IPv6 header */ >>>> + ODP_PARSE(IPFRAG), /* Parse IPv4 header if fragmented */ >>>> + ODP_PARSE(IPOPT), /* Parse IP options */ >>>> + ODP_PARSE(IPSEC), /* Parse IPsec header */ >>>> + ODP_PARSE(UDP), /* Parse UDP header */ >>>> + ODP_PARSE(TCP), /* Parse TCP header */ >>>> + ODP_PARSE(SCTP), /* Parse SCTP header */ >>>> + ODP_PARSE(ICMP), /* Parse ICMP header */ >>>> + ODP_PARSE_ALL = UINT32_MAX /* The application wants full parsing >> */ >>>> +} odp_packet_parse_e; >>>> +/** >>>> * @} >>>> */ >>>> >>>> diff --git a/include/odp/api/packet_io.h b/include/odp/api/packet_io.h >>>> index 6d31aeb..8c989f3 100644 >>>> --- a/include/odp/api/packet_io.h >>>> +++ b/include/odp/api/packet_io.h >>>> @@ -51,9 +51,14 @@ extern "C" { >>>> * open device will fail, returning ODP_PKTIO_INVALID with errno set. >>>> * odp_pktio_lookup() may be used to obtain a handle to an already >> open >>>> device. >>>> * >>>> - * @param dev Packet IO device name >>>> - * @param pool Pool from which to allocate buffers for storing >> packets >>>> - * received over this packet IO >>>> + * @param dev Packet IO device name >>>> + * @param pool Pool from which to allocate buffers for >> storing >>>> packets >>>> + * received over this packet IO >>>> + * @param parsing_mask Mask to request parsing. Must be one or more >>>> ODP_PARSE_* >>>> + * value joined with bitwise OR to request >>>> particular >>>> + * fields to be parsed. Or one of two special >>>> values: >>>> + * ODP_PARSE_NONE or ODP_PARSE_ALL. See >>>> odp_packet_parse_e >>>> + * in packet_flags.h for more details >>>> * >>>> * @return ODP packet IO handle >>>> * @retval ODP_PKTIO_INVALID on failure >>>> @@ -62,7 +67,8 @@ extern "C" { >>>> * device used for testing. Usually it's loop back >>>> * interface. >>>> */ >>>> -odp_pktio_t odp_pktio_open(const char *dev, odp_pool_t pool); >>>> +odp_pktio_t odp_pktio_open(const char *dev, odp_pool_t pool, >>>> + uint32_t parsing_mask); >>>> >>>> /** >>>> * Close an ODP packet IO instance >>>> -- >>>> 1.9.1 >>>> >>>> _______________________________________________ >>>> lng-odp mailing list >>>> lng-odp@lists.linaro.org >>>> https://lists.linaro.org/mailman/listinfo/lng-odp
How that supposed to increase performance?
The code will be like:
if (parse_vlan)
do_parse_vlan
if (parse_ipv4)
do_parse_ipv4
and etc. I.e. You will add about 20-30 if branches.
I think that this code is not sufficient. And parsing has to be under
#ifdefs because it's critical
for performance per packet functions.
Best regards,
Maxim.
On 04/17/15 14:07, Zoltan Kiss wrote:
>
>
> On 17/04/15 07:53, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>>
>>
>>> -----Original Message-----
>>> From: ext Zoltan Kiss [mailto:zoltan.kiss@linaro.org]
>>> Sent: Thursday, April 16, 2015 11:05 PM
>>> To: Savolainen, Petri (Nokia - FI/Espoo); lng-odp@lists.linaro.org
>>> Subject: Re: [lng-odp] [RFC API-NEXT PATCH] packet_io: add bitmap to
>>> specify what level of parsing needed
>>>
>>>
>>>
>>> On 16/04/15 13:33, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>>>> Hi,
>>>>
>>>> I think this should be typed (as bit field) and part of the
>>> odp_pktio_param_t params that I introduced in patch "api: packet_io:
>>> added
>>> odp_pktio_param_t". I could rework those patches and add it there.
>>> Ok, that would be probably even better.
>>>
>>>>
>>>> Something like this,
>>>>
>>>> typedef struct odp_pktio_input_flags_t {
>>>> struct {
>>>> uint64_t eth:1;
>>>> uint64_t jumbo:1;
>>>> uint64_t vlan:1;
>>>> ...
>>>>
>>>> uint64_t _reserved:27;
>>>> };
>>>> } odp_pktio_input_flags_t;
>>> I think it would be better to have a name which contains "parse" in
>>> some
>>> way.
>>
>> This same definition could be reused somewhere else in the API ...
>>
>>>
>>>>
>>>>
>>>> typedef struct odp_pktio_param_t {
>>>> /** Packet input mode */
>>>> enum odp_pktio_input_mode in_mode;
>>>> /** Packet input parser flags */
>>>> odp_pktio_input_flags_t flags;
>>
>> ... but here it could be contained.
>>
>> odp_pktio_input_flags_t parse;
>
> Ok, sounds good.
>>
>> -petri
>>
>>>> } odp_pktio_param_t;
>>>>
>>>>
>>>> odp_pktio_t odp_pktio_open(const char *dev, odp_pool_t pool,
>>>> const odp_pktio_param_t *param);
>>>>
>>>>
>>>> -Petri
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of
>>> ext
>>>>> Zoltan Kiss
>>>>> Sent: Wednesday, April 15, 2015 7:01 PM
>>>>> To: lng-odp@lists.linaro.org
>>>>> Subject: [lng-odp] [RFC API-NEXT PATCH] packet_io: add bitmap to
>>> specify
>>>>> what level of parsing needed
>>>>>
>>>>> odp_pktio_open() will have a 32 bit bitmask to specify what kind of
>>> header
>>>>> parsing is required by the application.
>>>>>
>>>>> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org>
>>>>> ---
>>>>> include/odp/api/packet_flags.h | 49
>>>>> ++++++++++++++++++++++++++++++++++++++++++
>>>>> include/odp/api/packet_io.h | 14 ++++++++----
>>>>> 2 files changed, 59 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/include/odp/api/packet_flags.h
>>>>> b/include/odp/api/packet_flags.h
>>>>> index b1e179e..467d4f1 100644
>>>>> --- a/include/odp/api/packet_flags.h
>>>>> +++ b/include/odp/api/packet_flags.h
>>>>> @@ -327,6 +327,55 @@ void odp_packet_has_sctp_set(odp_packet_t
>>>>> pkt, int
>>>>> val);
>>>>> void odp_packet_has_icmp_set(odp_packet_t pkt, int val);
>>>>>
>>>>> /**
>>>>> + * Shift values for enum odp_packet_parse. Has to be updated
>>>>> together
>>>>> with
>>>>> + * odp_packet_parse_e type. The parameter passed to
>>>>> odp_pktio_open is
>>> a
>>>>> 32 bit
>>>>> + * mask.
>>>>> + */
>>>>> +typedef enum odp_packet_parse_shift {
>>>>> + ODP_PARSE_SHIFT_ETH,
>>>>> + ODP_PARSE_SHIFT_JUMBO,
>>>>> + ODP_PARSE_SHIFT_VLAN,
>>>>> + ODP_PARSE_SHIFT_VLAN_QINQ,
>>>>> + ODP_PARSE_SHIFT_ARP,
>>>>> + ODP_PARSE_SHIFT_IPV4,
>>>>> + ODP_PARSE_SHIFT_IPV6,
>>>>> + ODP_PARSE_SHIFT_IPFRAG,
>>>>> + ODP_PARSE_SHIFT_IPOPT,
>>>>> + ODP_PARSE_SHIFT_IPSEC,
>>>>> + ODP_PARSE_SHIFT_UDP,
>>>>> + ODP_PARSE_SHIFT_TCP,
>>>>> + ODP_PARSE_SHIFT_SCTP,
>>>>> + ODP_PARSE_SHIFT_ICMP,
>>>>> + ODP_PARSE_SHIFT_MAX = 31
>>>>> +} odp_packet_parse_shift_e;
>>>>> +
>>>>> +#define ODP_PARSE(FIELD) ODP_PARSE_##FIELD =
>>>>> (ODP_PARSE_SHIFT_##FIELD
>>> <<
>>>>> 1)
>>>>> +
>>>>> +/**
>>>>> + * Values to be used when calling odp_pktio_open. The parser_mask
>>>>> parameter has
>>>>> + * to be one or more of these values joined with bitwise OR. Or
>>>>> one of
>>>>> the two
>>>>> + * special values: ODP_PARSE_NONE or ODP_PARSE_ALL.
>>>>> + * Has to be updated together with odp_packet_parse_shift_e
>>>>> + */
>>>>> +typedef enum odp_packet_parse {
>>>>> + ODP_PARSE_NONE = 0, /* The application don't want any
>>> parsing */
>>>>> + ODP_PARSE(ETH), /* Parse Ethernet header */
>>>>> + ODP_PARSE(JUMBO), /* Parse Ethernet header if a jumbo
>>>>> frame */
>>>>> + ODP_PARSE(VLAN), /* Parse VLAN header */
>>>>> + ODP_PARSE(VLAN_QINQ), /* Parse VLAN QinQ header */
>>>>> + ODP_PARSE(ARP), /* Parse ARP header */
>>>>> + ODP_PARSE(IPV4), /* Parse IPv4 header */
>>>>> + ODP_PARSE(IPV6), /* Parse IPv6 header */
>>>>> + ODP_PARSE(IPFRAG), /* Parse IPv4 header if fragmented */
>>>>> + ODP_PARSE(IPOPT), /* Parse IP options */
>>>>> + ODP_PARSE(IPSEC), /* Parse IPsec header */
>>>>> + ODP_PARSE(UDP), /* Parse UDP header */
>>>>> + ODP_PARSE(TCP), /* Parse TCP header */
>>>>> + ODP_PARSE(SCTP), /* Parse SCTP header */
>>>>> + ODP_PARSE(ICMP), /* Parse ICMP header */
>>>>> + ODP_PARSE_ALL = UINT32_MAX /* The application wants full
>>>>> parsing
>>> */
>>>>> +} odp_packet_parse_e;
>>>>> +/**
>>>>> * @}
>>>>> */
>>>>>
>>>>> diff --git a/include/odp/api/packet_io.h
>>>>> b/include/odp/api/packet_io.h
>>>>> index 6d31aeb..8c989f3 100644
>>>>> --- a/include/odp/api/packet_io.h
>>>>> +++ b/include/odp/api/packet_io.h
>>>>> @@ -51,9 +51,14 @@ extern "C" {
>>>>> * open device will fail, returning ODP_PKTIO_INVALID with
>>>>> errno set.
>>>>> * odp_pktio_lookup() may be used to obtain a handle to an already
>>> open
>>>>> device.
>>>>> *
>>>>> - * @param dev Packet IO device name
>>>>> - * @param pool Pool from which to allocate buffers for storing
>>> packets
>>>>> - * received over this packet IO
>>>>> + * @param dev Packet IO device name
>>>>> + * @param pool Pool from which to allocate buffers for
>>> storing
>>>>> packets
>>>>> + * received over this packet IO
>>>>> + * @param parsing_mask Mask to request parsing. Must be one or more
>>>>> ODP_PARSE_*
>>>>> + * value joined with bitwise OR to request
>>>>> particular
>>>>> + * fields to be parsed. Or one of two special
>>>>> values:
>>>>> + * ODP_PARSE_NONE or ODP_PARSE_ALL. See
>>>>> odp_packet_parse_e
>>>>> + * in packet_flags.h for more details
>>>>> *
>>>>> * @return ODP packet IO handle
>>>>> * @retval ODP_PKTIO_INVALID on failure
>>>>> @@ -62,7 +67,8 @@ extern "C" {
>>>>> * device used for testing. Usually it's loop back
>>>>> * interface.
>>>>> */
>>>>> -odp_pktio_t odp_pktio_open(const char *dev, odp_pool_t pool);
>>>>> +odp_pktio_t odp_pktio_open(const char *dev, odp_pool_t pool,
>>>>> + uint32_t parsing_mask);
>>>>>
>>>>> /**
>>>>> * Close an ODP packet IO instance
>>>>> --
>>>>> 1.9.1
>>>>>
>>>>> _______________________________________________
>>>>> 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
On 20 April 2015 at 11:12, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > How that supposed to increase performance? > > The code will be like: > > if (parse_vlan) > do_parse_vlan > if (parse_ipv4) > do_parse_ipv4 > > and etc. I.e. You will add about 20-30 if branches. > That's exactly how you are *not* supposed to implemented the parsing and that is the coding I wanted to avoid with specifying the layer instead of individual protocols. But as I then suggested, an implementation can set the parsing levels based on the individual flags. if (pktio->parse_layer_2) { //Unconditional parsing of Ethernet/VLAN etc .... if (pkt->parse_layer_3) { //Unconditional parsing of IPv4/IPv6 headers .... if (pkt->parse_layer_4) { //Unconditional parsing of e.g. UDP/TCP/SCTP headers } } } > I think that this code is not sufficient. And parsing has to be under > #ifdefs because it's critical > for performance per packet functions. > If the application does not specify any parsing flags at all, the first test (if (parse_layer_2)) will fail and no further parsing code will be executed. The branch predictor will short-circuit the conditional branch. There will be a function call (also handled by the branch predictor). I doubt the cycle overhead of a function call and a failed test will be very large (10-20 cycles?). The current parsing code is not super optimized. It would be an interesting micro project to profile and optimize if for some common configurations and workloads (e.g. no parsing at all, some parsing for IPv4 traffic). We might not want that code in linux-generic. > Best regards, > Maxim. > > > > On 04/17/15 14:07, Zoltan Kiss wrote: > >> >> >> On 17/04/15 07:53, Savolainen, Petri (Nokia - FI/Espoo) wrote: >> >>> >>> >>> -----Original Message----- >>>> From: ext Zoltan Kiss [mailto:zoltan.kiss@linaro.org] >>>> Sent: Thursday, April 16, 2015 11:05 PM >>>> To: Savolainen, Petri (Nokia - FI/Espoo); lng-odp@lists.linaro.org >>>> Subject: Re: [lng-odp] [RFC API-NEXT PATCH] packet_io: add bitmap to >>>> specify what level of parsing needed >>>> >>>> >>>> >>>> On 16/04/15 13:33, Savolainen, Petri (Nokia - FI/Espoo) wrote: >>>> >>>>> Hi, >>>>> >>>>> I think this should be typed (as bit field) and part of the >>>>> >>>> odp_pktio_param_t params that I introduced in patch "api: packet_io: >>>> added >>>> odp_pktio_param_t". I could rework those patches and add it there. >>>> Ok, that would be probably even better. >>>> >>>> >>>>> Something like this, >>>>> >>>>> typedef struct odp_pktio_input_flags_t { >>>>> struct { >>>>> uint64_t eth:1; >>>>> uint64_t jumbo:1; >>>>> uint64_t vlan:1; >>>>> ... >>>>> >>>>> uint64_t _reserved:27; >>>>> }; >>>>> } odp_pktio_input_flags_t; >>>>> >>>> I think it would be better to have a name which contains "parse" in some >>>> way. >>>> >>> >>> This same definition could be reused somewhere else in the API ... >>> >>> >>>> >>>>> >>>>> typedef struct odp_pktio_param_t { >>>>> /** Packet input mode */ >>>>> enum odp_pktio_input_mode in_mode; >>>>> /** Packet input parser flags */ >>>>> odp_pktio_input_flags_t flags; >>>>> >>>> >>> ... but here it could be contained. >>> >>> odp_pktio_input_flags_t parse; >>> >> >> Ok, sounds good. >> >>> >>> -petri >>> >>> } odp_pktio_param_t; >>>>> >>>>> >>>>> odp_pktio_t odp_pktio_open(const char *dev, odp_pool_t pool, >>>>> const odp_pktio_param_t *param); >>>>> >>>>> >>>>> -Petri >>>>> >>>>> >>>>> -----Original Message----- >>>>>> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of >>>>>> >>>>> ext >>>> >>>>> Zoltan Kiss >>>>>> Sent: Wednesday, April 15, 2015 7:01 PM >>>>>> To: lng-odp@lists.linaro.org >>>>>> Subject: [lng-odp] [RFC API-NEXT PATCH] packet_io: add bitmap to >>>>>> >>>>> specify >>>> >>>>> what level of parsing needed >>>>>> >>>>>> odp_pktio_open() will have a 32 bit bitmask to specify what kind of >>>>>> >>>>> header >>>> >>>>> parsing is required by the application. >>>>>> >>>>>> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org> >>>>>> --- >>>>>> include/odp/api/packet_flags.h | 49 >>>>>> ++++++++++++++++++++++++++++++++++++++++++ >>>>>> include/odp/api/packet_io.h | 14 ++++++++---- >>>>>> 2 files changed, 59 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/include/odp/api/packet_flags.h >>>>>> b/include/odp/api/packet_flags.h >>>>>> index b1e179e..467d4f1 100644 >>>>>> --- a/include/odp/api/packet_flags.h >>>>>> +++ b/include/odp/api/packet_flags.h >>>>>> @@ -327,6 +327,55 @@ void odp_packet_has_sctp_set(odp_packet_t pkt, >>>>>> int >>>>>> val); >>>>>> void odp_packet_has_icmp_set(odp_packet_t pkt, int val); >>>>>> >>>>>> /** >>>>>> + * Shift values for enum odp_packet_parse. Has to be updated together >>>>>> with >>>>>> + * odp_packet_parse_e type. The parameter passed to odp_pktio_open is >>>>>> >>>>> a >>>> >>>>> 32 bit >>>>>> + * mask. >>>>>> + */ >>>>>> +typedef enum odp_packet_parse_shift { >>>>>> + ODP_PARSE_SHIFT_ETH, >>>>>> + ODP_PARSE_SHIFT_JUMBO, >>>>>> + ODP_PARSE_SHIFT_VLAN, >>>>>> + ODP_PARSE_SHIFT_VLAN_QINQ, >>>>>> + ODP_PARSE_SHIFT_ARP, >>>>>> + ODP_PARSE_SHIFT_IPV4, >>>>>> + ODP_PARSE_SHIFT_IPV6, >>>>>> + ODP_PARSE_SHIFT_IPFRAG, >>>>>> + ODP_PARSE_SHIFT_IPOPT, >>>>>> + ODP_PARSE_SHIFT_IPSEC, >>>>>> + ODP_PARSE_SHIFT_UDP, >>>>>> + ODP_PARSE_SHIFT_TCP, >>>>>> + ODP_PARSE_SHIFT_SCTP, >>>>>> + ODP_PARSE_SHIFT_ICMP, >>>>>> + ODP_PARSE_SHIFT_MAX = 31 >>>>>> +} odp_packet_parse_shift_e; >>>>>> + >>>>>> +#define ODP_PARSE(FIELD) ODP_PARSE_##FIELD = (ODP_PARSE_SHIFT_##FIELD >>>>>> >>>>> << >>>> >>>>> 1) >>>>>> + >>>>>> +/** >>>>>> + * Values to be used when calling odp_pktio_open. The parser_mask >>>>>> parameter has >>>>>> + * to be one or more of these values joined with bitwise OR. Or one >>>>>> of >>>>>> the two >>>>>> + * special values: ODP_PARSE_NONE or ODP_PARSE_ALL. >>>>>> + * Has to be updated together with odp_packet_parse_shift_e >>>>>> + */ >>>>>> +typedef enum odp_packet_parse { >>>>>> + ODP_PARSE_NONE = 0, /* The application don't want any >>>>>> >>>>> parsing */ >>>> >>>>> + ODP_PARSE(ETH), /* Parse Ethernet header */ >>>>>> + ODP_PARSE(JUMBO), /* Parse Ethernet header if a jumbo frame >>>>>> */ >>>>>> + ODP_PARSE(VLAN), /* Parse VLAN header */ >>>>>> + ODP_PARSE(VLAN_QINQ), /* Parse VLAN QinQ header */ >>>>>> + ODP_PARSE(ARP), /* Parse ARP header */ >>>>>> + ODP_PARSE(IPV4), /* Parse IPv4 header */ >>>>>> + ODP_PARSE(IPV6), /* Parse IPv6 header */ >>>>>> + ODP_PARSE(IPFRAG), /* Parse IPv4 header if fragmented */ >>>>>> + ODP_PARSE(IPOPT), /* Parse IP options */ >>>>>> + ODP_PARSE(IPSEC), /* Parse IPsec header */ >>>>>> + ODP_PARSE(UDP), /* Parse UDP header */ >>>>>> + ODP_PARSE(TCP), /* Parse TCP header */ >>>>>> + ODP_PARSE(SCTP), /* Parse SCTP header */ >>>>>> + ODP_PARSE(ICMP), /* Parse ICMP header */ >>>>>> + ODP_PARSE_ALL = UINT32_MAX /* The application wants full parsing >>>>>> >>>>> */ >>>> >>>>> +} odp_packet_parse_e; >>>>>> +/** >>>>>> * @} >>>>>> */ >>>>>> >>>>>> diff --git a/include/odp/api/packet_io.h b/include/odp/api/packet_io.h >>>>>> index 6d31aeb..8c989f3 100644 >>>>>> --- a/include/odp/api/packet_io.h >>>>>> +++ b/include/odp/api/packet_io.h >>>>>> @@ -51,9 +51,14 @@ extern "C" { >>>>>> * open device will fail, returning ODP_PKTIO_INVALID with errno >>>>>> set. >>>>>> * odp_pktio_lookup() may be used to obtain a handle to an already >>>>>> >>>>> open >>>> >>>>> device. >>>>>> * >>>>>> - * @param dev Packet IO device name >>>>>> - * @param pool Pool from which to allocate buffers for storing >>>>>> >>>>> packets >>>> >>>>> - * received over this packet IO >>>>>> + * @param dev Packet IO device name >>>>>> + * @param pool Pool from which to allocate buffers for >>>>>> >>>>> storing >>>> >>>>> packets >>>>>> + * received over this packet IO >>>>>> + * @param parsing_mask Mask to request parsing. Must be one or more >>>>>> ODP_PARSE_* >>>>>> + * value joined with bitwise OR to request >>>>>> particular >>>>>> + * fields to be parsed. Or one of two special >>>>>> values: >>>>>> + * ODP_PARSE_NONE or ODP_PARSE_ALL. See >>>>>> odp_packet_parse_e >>>>>> + * in packet_flags.h for more details >>>>>> * >>>>>> * @return ODP packet IO handle >>>>>> * @retval ODP_PKTIO_INVALID on failure >>>>>> @@ -62,7 +67,8 @@ extern "C" { >>>>>> * device used for testing. Usually it's loop back >>>>>> * interface. >>>>>> */ >>>>>> -odp_pktio_t odp_pktio_open(const char *dev, odp_pool_t pool); >>>>>> +odp_pktio_t odp_pktio_open(const char *dev, odp_pool_t pool, >>>>>> + uint32_t parsing_mask); >>>>>> >>>>>> /** >>>>>> * Close an ODP packet IO instance >>>>>> -- >>>>>> 1.9.1 >>>>>> >>>>>> _______________________________________________ >>>>>> 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 >
On 20/04/15 10:36, Ola Liljedahl wrote: > On 20 April 2015 at 11:12, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > How that supposed to increase performance? > > The code will be like: > > if (parse_vlan) > do_parse_vlan > if (parse_ipv4) > do_parse_ipv4 > > and etc. I.e. You will add about 20-30 if branches. > > That's exactly how you are *not* supposed to implemented the parsing and > that is the coding I wanted to avoid with specifying the layer instead > of individual protocols. But as I then suggested, an implementation can > set the parsing levels based on the individual flags. > > if (pktio->parse_layer_2) { > //Unconditional parsing of Ethernet/VLAN etc > .... > if (pkt->parse_layer_3) { > //Unconditional parsing of IPv4/IPv6 headers > .... > if (pkt->parse_layer_4) { > //Unconditional parsing of e.g. UDP/TCP/SCTP headers > } > } > } > That would work with a more detailed parsing configuration as well: if (pktio->eth) && ((pktio->jumbo) || (frame_len < 1524)) && ((pktio->vlan) || (ethertype != 0x8100)) && ... And the branch predictor can optimize this out as well. Zoli
I think the point is that decoding what the application has specified likely costs about as much as just doing the parse to begin with if you're doing this in SW, and HW probably doesn't have anything like this sort of control. So what is the point here? A "lazy parser" approach would seem to be both simpler for the application and potentially faster for the implementation. But some minimal level of parsing is needed if only to identify the layer offsets. The only significant speedup you're going to see is if SW parsing (in ODP) is bypassed entirely because the implementation is doing it all on its own. On Tue, Apr 21, 2015 at 8:38 AM, Zoltan Kiss <zoltan.kiss@linaro.org> wrote: > > > On 20/04/15 10:36, Ola Liljedahl wrote: > >> On 20 April 2015 at 11:12, Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> wrote: >> >> How that supposed to increase performance? >> >> The code will be like: >> >> if (parse_vlan) >> do_parse_vlan >> if (parse_ipv4) >> do_parse_ipv4 >> >> and etc. I.e. You will add about 20-30 if branches. >> >> That's exactly how you are *not* supposed to implemented the parsing and >> that is the coding I wanted to avoid with specifying the layer instead >> of individual protocols. But as I then suggested, an implementation can >> set the parsing levels based on the individual flags. >> >> if (pktio->parse_layer_2) { >> //Unconditional parsing of Ethernet/VLAN etc >> .... >> if (pkt->parse_layer_3) { >> //Unconditional parsing of IPv4/IPv6 headers >> .... >> if (pkt->parse_layer_4) { >> //Unconditional parsing of e.g. UDP/TCP/SCTP headers >> } >> } >> } >> >> > That would work with a more detailed parsing configuration as well: > > > if (pktio->eth) && > ((pktio->jumbo) || (frame_len < 1524)) && > ((pktio->vlan) || (ethertype != 0x8100)) && > ... > > And the branch predictor can optimize this out as well. > > Zoli > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
diff --git a/include/odp/api/packet_flags.h b/include/odp/api/packet_flags.h index b1e179e..467d4f1 100644 --- a/include/odp/api/packet_flags.h +++ b/include/odp/api/packet_flags.h @@ -327,6 +327,55 @@ void odp_packet_has_sctp_set(odp_packet_t pkt, int val); void odp_packet_has_icmp_set(odp_packet_t pkt, int val); /** + * Shift values for enum odp_packet_parse. Has to be updated together with + * odp_packet_parse_e type. The parameter passed to odp_pktio_open is a 32 bit + * mask. + */ +typedef enum odp_packet_parse_shift { + ODP_PARSE_SHIFT_ETH, + ODP_PARSE_SHIFT_JUMBO, + ODP_PARSE_SHIFT_VLAN, + ODP_PARSE_SHIFT_VLAN_QINQ, + ODP_PARSE_SHIFT_ARP, + ODP_PARSE_SHIFT_IPV4, + ODP_PARSE_SHIFT_IPV6, + ODP_PARSE_SHIFT_IPFRAG, + ODP_PARSE_SHIFT_IPOPT, + ODP_PARSE_SHIFT_IPSEC, + ODP_PARSE_SHIFT_UDP, + ODP_PARSE_SHIFT_TCP, + ODP_PARSE_SHIFT_SCTP, + ODP_PARSE_SHIFT_ICMP, + ODP_PARSE_SHIFT_MAX = 31 +} odp_packet_parse_shift_e; + +#define ODP_PARSE(FIELD) ODP_PARSE_##FIELD = (ODP_PARSE_SHIFT_##FIELD << 1) + +/** + * Values to be used when calling odp_pktio_open. The parser_mask parameter has + * to be one or more of these values joined with bitwise OR. Or one of the two + * special values: ODP_PARSE_NONE or ODP_PARSE_ALL. + * Has to be updated together with odp_packet_parse_shift_e + */ +typedef enum odp_packet_parse { + ODP_PARSE_NONE = 0, /* The application don't want any parsing */ + ODP_PARSE(ETH), /* Parse Ethernet header */ + ODP_PARSE(JUMBO), /* Parse Ethernet header if a jumbo frame */ + ODP_PARSE(VLAN), /* Parse VLAN header */ + ODP_PARSE(VLAN_QINQ), /* Parse VLAN QinQ header */ + ODP_PARSE(ARP), /* Parse ARP header */ + ODP_PARSE(IPV4), /* Parse IPv4 header */ + ODP_PARSE(IPV6), /* Parse IPv6 header */ + ODP_PARSE(IPFRAG), /* Parse IPv4 header if fragmented */ + ODP_PARSE(IPOPT), /* Parse IP options */ + ODP_PARSE(IPSEC), /* Parse IPsec header */ + ODP_PARSE(UDP), /* Parse UDP header */ + ODP_PARSE(TCP), /* Parse TCP header */ + ODP_PARSE(SCTP), /* Parse SCTP header */ + ODP_PARSE(ICMP), /* Parse ICMP header */ + ODP_PARSE_ALL = UINT32_MAX /* The application wants full parsing */ +} odp_packet_parse_e; +/** * @} */ diff --git a/include/odp/api/packet_io.h b/include/odp/api/packet_io.h index 6d31aeb..8c989f3 100644 --- a/include/odp/api/packet_io.h +++ b/include/odp/api/packet_io.h @@ -51,9 +51,14 @@ extern "C" { * open device will fail, returning ODP_PKTIO_INVALID with errno set. * odp_pktio_lookup() may be used to obtain a handle to an already open device. * - * @param dev Packet IO device name - * @param pool Pool from which to allocate buffers for storing packets - * received over this packet IO + * @param dev Packet IO device name + * @param pool Pool from which to allocate buffers for storing packets + * received over this packet IO + * @param parsing_mask Mask to request parsing. Must be one or more ODP_PARSE_* + * value joined with bitwise OR to request particular + * fields to be parsed. Or one of two special values: + * ODP_PARSE_NONE or ODP_PARSE_ALL. See odp_packet_parse_e + * in packet_flags.h for more details * * @return ODP packet IO handle * @retval ODP_PKTIO_INVALID on failure @@ -62,7 +67,8 @@ extern "C" { * device used for testing. Usually it's loop back * interface. */ -odp_pktio_t odp_pktio_open(const char *dev, odp_pool_t pool); +odp_pktio_t odp_pktio_open(const char *dev, odp_pool_t pool, + uint32_t parsing_mask); /** * Close an ODP packet IO instance
odp_pktio_open() will have a 32 bit bitmask to specify what kind of header parsing is required by the application. Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org> --- include/odp/api/packet_flags.h | 49 ++++++++++++++++++++++++++++++++++++++++++ include/odp/api/packet_io.h | 14 ++++++++---- 2 files changed, 59 insertions(+), 4 deletions(-)