On 20 April 2015 at 11:12, Maxim Uvarov <[email protected]> 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:[email protected]]
>>>> Sent: Thursday, April 16, 2015 11:05 PM
>>>> To: Savolainen, Petri (Nokia - FI/Espoo); [email protected]
>>>> 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:[email protected]] On Behalf Of
>>>>>>
>>>>> ext
>>>>
>>>>> Zoltan Kiss
>>>>>> Sent: Wednesday, April 15, 2015 7:01 PM
>>>>>> To: [email protected]
>>>>>> 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 <[email protected]>
>>>>>> ---
>>>>>> 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
>>>>>> [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
