Dmitry Eremin-Solenikov(lumag) replied on github web page:
include/odp/api/spec/packet.h
line 75
@@ -1140,6 +1179,82 @@ int odp_packet_move_data(odp_packet_t pkt, uint32_t
dst_offset,
*/
/**
+ * Packet parse parameters
+ */
+typedef struct odp_packet_parse_param_t {
+ /** Protocol header at parse starting point. Valid values for this
+ * field are: ODP_PROTO_ETH, ODP_PROTO_IPV4, ODP_PROTO_IPV6. */
+ odp_proto_t proto;
+
+ /** Continue parsing until this layer. Must be the same or higher
+ * layer than the layer of 'proto'. */
+ odp_proto_layer_t layer;
+
+ /** Flags to control payload data checks up to the selected parse
+ * layer. Checksum checking status can be queried for each packet with
+ * odp_packet_l3_chksum_status() and odp_packet_l4_chksum_status().
+ */
+ union {
+ struct {
+ /** Check IPv4 header checksum */
+ uint32_t ipv4_chksum : 1;
+
+ /** Check UDP checksum */
+ uint32_t udp_chksum : 1;
+
+ /** Check TCP checksum */
+ uint32_t tcp_chksum : 1;
Comment:
@psavol ok, agreed
> Petri Savolainen(psavol) wrote:
> This function is different from e.g. enqueue multi, where the destination
> queue may be close to full and that's why only part of events were enqueued.
> Here operation fails due to bad params or internal error.
>
> Anyway, I'll change it to return num processed. It's more flexible.
>> bogdanPricope wrote
>> What about: "When operation fails, metadata of checked layers indicates the
>> error condition."?
>>> bogdanPricope wrote
>>> What about: "Metadata of other layers remains unmodified."?
>>>> Petri Savolainen(psavol) wrote:
>>>> Layer is not enough information for the parser. It needs to know the first
>>>> protocol (e.g. MPLS vs IP) to be able to start the parsing.
>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>> @psavol `_param_init` seems like an overkill in this case. From my point
>>>>> of view, `param_init` are good for object creation params (where we might
>>>>> not know platform-optimized defaults) but are an overkill for operation
>>>>> functions (we do not have `param_init` for ipsec, crypto, etc operational
>>>>> params).
>>>>>> Petri Savolainen(psavol) wrote:
>>>>>> No, other APIs order checksum checking the same way: per protocol.
>>>>>> Application may be e.g. terminating only UDP and forwarding all others -
>>>>>> so it would be waste to check e.g. TCP checksum.
>>>>>>
>>>>>> typedef union odp_pktin_config_opt_t {
>>>>>> /** Option flags */
>>>>>> struct {
>>>>>> /** Timestamp all packets on packet input */
>>>>>> uint64_t ts_all : 1;
>>>>>>
>>>>>> /** Timestamp (at least) IEEE1588 / PTP packets
>>>>>> * on packet input */
>>>>>> uint64_t ts_ptp : 1;
>>>>>>
>>>>>> /** Check IPv4 header checksum on packet input */
>>>>>> uint64_t ipv4_chksum : 1;
>>>>>>
>>>>>> /** Check UDP checksum on packet input */
>>>>>> uint64_t udp_chksum : 1;
>>>>>>
>>>>>> /** Check TCP checksum on packet input */
>>>>>> uint64_t tcp_chksum : 1;
>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>> Param init on fast path is a bit overkill. However, application could
>>>>>>> call it once and store the result. So, it could avoid extra function
>>>>>>> call for every packet. So, I'll add it.
>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>> `return i;` if we adopt the RC == number of successful packets parsed
>>>>>>>> convention.
>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>> For consistency with other `odp_xxx_multi()` APIs, the RC should
>>>>>>>>> indicate the number of input packets that were successfully
>>>>>>>>> processed.
>>>>>>>>>> muvarov wrote
>>>>>>>>>> it looks like 2 enums are not needed here. odp_proto_layet_t
>>>>>>>>>> layer_start; and odp_proto_layet_t layer_end; and one enum for
>>>>>>>>>> layers.
>>>>>>>>>>> muvarov wrote
>>>>>>>>>>> maybe return number of correctly parsed packets? And say that
>>>>>>>>>>> parsing always starts from be beginning of array.
>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>> +1 since other checksum APIs refer to L3 and L4 rather than
>>>>>>>>>>>> specific protocols.
>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>> What about just `l3_chksum`, and `l4_chksum`?
>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>> Now that we have a `param` struct we should have an
>>>>>>>>>>>>>> `odp_packet_parse_param_init()` API as well for completeness.
>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>> @Bill-Fischofer-Linaro Because it is a parsing error, if lower
>>>>>>>>>>>>>>> layer provides us IP version that does not correspond to the
>>>>>>>>>>>>>>> in-packet version. We should detect that, rather than silently
>>>>>>>>>>>>>>> parsing this header.
>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>> I'm not sure why an API named `odp_packet_parse()` needs help
>>>>>>>>>>>>>>>> here. It's purpose, after all, is to parse packets and
>>>>>>>>>>>>>>>> determining IPv4 vs IPv6 is part of that activity. Moreover,
>>>>>>>>>>>>>>>> the only way an application can inspect the IP header is to
>>>>>>>>>>>>>>>> access it via other ODP API calls, so I don't see how asking
>>>>>>>>>>>>>>>> the application to do this is any better than having the
>>>>>>>>>>>>>>>> `odp_packet_parse()` implementation do this itself. What's the
>>>>>>>>>>>>>>>> purpose of having a parse API in that case since clearly the
>>>>>>>>>>>>>>>> application could parse the entire packet "by hand" as well.
>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>> Failure needs to be defined in a more precise way (and maybe
>>>>>>>>>>>>>>>>> for a single packet case). I assume that it means internal
>>>>>>>>>>>>>>>>> ODP error, rather than just packet with wrong headers. What
>>>>>>>>>>>>>>>>> happens in multi-packet case if failure occurs in the middle
>>>>>>>>>>>>>>>>> of parsing?
>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>> @psavol Also for multi-packet parsing, we can change `proto`
>>>>>>>>>>>>>>>>>> to be an array, allowing applications to easily intermix
>>>>>>>>>>>>>>>>>> IPv4 and IPv6 packets.
>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>> @psavol yes. I just wanted to focus on cases, when passing
>>>>>>>>>>>>>>>>>>> packet with wrong protocol is an error. E.g. IPv6 packet
>>>>>>>>>>>>>>>>>>> inside IPsec packet with NH = 4. So it is not a question of
>>>>>>>>>>>>>>>>>>> selecting proper L3 parser, but rather a question of
>>>>>>>>>>>>>>>>>>> nailing down error/malicious packets.
>>>>>>>>>>>>>>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>>>>>>>>>>>>>>> It's there to enable application to call parsing in parts
>>>>>>>>>>>>>>>>>>>> - e.g. first up to IP and then continue from L4. But since
>>>>>>>>>>>>>>>>>>>> IP and transport protocols are tied together with pseudo
>>>>>>>>>>>>>>>>>>>> headers, it's cleaner to remove L4 as a starting point.
>>>>>>>>>>>>>>>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>>>>>>>>>>>>>>>> First bits of an IP header marks the version. So, it
>>>>>>>>>>>>>>>>>>>>> would be trivial for both app and implementation to read
>>>>>>>>>>>>>>>>>>>>> the version from the data. Common IP define is easier for
>>>>>>>>>>>>>>>>>>>>> application when a burst of packets may contain both v4
>>>>>>>>>>>>>>>>>>>>> and v6 mixed. Application does not need to sort packets
>>>>>>>>>>>>>>>>>>>>> into two arrays (one for v4 and other for v6) but just
>>>>>>>>>>>>>>>>>>>>> pass the entire array for parsing.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> There are three ways to define the enumeration: IP,
>>>>>>>>>>>>>>>>>>>>> IPv4+IPv6, IP+IPv4+IPv6. I'm OK with any of those.
>>>>>>>>>>>>>>>>>>>>> IPv4+IPv6 would be a bit more robust since version
>>>>>>>>>>>>>>>>>>>>> information comes from two sources.
>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>> I felt easier to reparse both L3 and L4 headers in IPsec
>>>>>>>>>>>>>>>>>>>>>> case, especially since transport mode ESP can en/decrypt
>>>>>>>>>>>>>>>>>>>>>> some of L3 headers in IPv6 case.
>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>> @Bill-Fischofer-Linaro In IPsec case Next Header field
>>>>>>>>>>>>>>>>>>>>>>> will contain 4 for IPv4 and 41 for IPv6.
>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>> That might be overly complicated since until a
>>>>>>>>>>>>>>>>>>>>>>>> decrypted tunnel mode IPsec packet is parsed you don't
>>>>>>>>>>>>>>>>>>>>>>>> know whether it's IPv4 or IPv6. It's parsing that
>>>>>>>>>>>>>>>>>>>>>>>> makes that determination.
>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>> @lumag IPsec operating in transport mode is, I'd
>>>>>>>>>>>>>>>>>>>>>>>>> imagine, the main use case here.
>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>> What is the usecase for parsing a packet starting
>>>>>>>>>>>>>>>>>>>>>>>>>> from L4 header? Also there are several (lots) of
>>>>>>>>>>>>>>>>>>>>>>>>>> other L4 protocols. Do we want to support them all
>>>>>>>>>>>>>>>>>>>>>>>>>> here?
>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>> Maybe it would be better to split this into
>>>>>>>>>>>>>>>>>>>>>>>>>>> separate IPv4 and IPv6 packets. It would be an
>>>>>>>>>>>>>>>>>>>>>>>>>>> error to pass IPv6 packet with ethtype (or IP
>>>>>>>>>>>>>>>>>>>>>>>>>>> tunnel type) being set to IPv4.
>>>>>>>>>>>>>>>>>>>>>>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>> A vector of packets is CPU vector instructions
>>>>>>>>>>>>>>>>>>>>>>>>>>>> friendly.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> The use case is mentioned in log message: parse
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> after decrypt/IP reassembly. Application has
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> recreated an inner packet and needs to parse it
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> before continue. This is inherently SW parse
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> which may be accelerated with CPU vector
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> instructions, etc.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> What's the use case for a multi() form of this
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> API? Might VPP use it? Perhaps Sachin can
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> comment?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> We had considered an `odp_packet_parse()`
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> function some time back but it was rejected as
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> something that would not fit well with hardware
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> parsers. What's changed?
https://github.com/Linaro/odp/pull/273#discussion_r149916437
updated_at 2017-11-09 12:49:44
