Petri Savolainen(psavol) replied on github web page:
include/odp/api/spec/packet.h
@@ -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;
+
+ } check;
+
+ /** All check bits. This can be used to set/clear all flags. */
+ uint32_t all_check;
+ };
+
+} odp_packet_parse_param_t;
+
+/**
+ * Parse packet
+ *
+ * Parse protocol headers in packet data. Parsing starts at 'offset', which
+ * is the first header byte of protocol 'param.proto'. Parameter 'param.layer'
+ * defines the last layer application is interested about.
+ * Use ODP_PROTO_LAYER_ALL for all layers. The operation sets or resets packet
+ * metadata for all layers from the layer of 'param.proto' to the application
+ * defined last layer. Metadata of other layers have undefined values.
Comment:
Application calls parse because metadata is not valid to begin with. There
would not be much benefit for application to require that implementation saves
old metadata values before the operation and then restores those after if e.g.
an internal error occurred. Implementation performance would be hurt by making
a copy of the old metadata.
Since it's just (L2/L3/L4) metadata (not data) that may have changed,
application can continue processing the packet data also after a failed parse.
> Dmitry Eremin-Solenikov(lumag) wrote:
> @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_r149916549
updated_at 2017-11-09 12:49:44
