bogdanPricope 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.
+ * When operation fails, metadata of all protocol layers have undefined values.
Comment:
OK
> bogdanPricope wrote
> Processing of UDP/TCP (in OFP and BSD like) depends on IPv4/IPv6: there are
> different input functions for UDP/TCP processing for IPv4 and IPv6
> (https://github.com/OpenFastPath/ofp/blob/master/src/ofp_in_proto.c,
> https://github.com/OpenFastPath/ofp/blob/master/src/ofp_in6_proto.c)
>> Petri Savolainen(psavol) wrote:
>> Then I don't add it now. We can add param init also later if param struct is
>> extended with other things than check flags. Todays spec defines already
>> that all flags should be set to zero.
>>
>> @param param Parse parameters. Proto and layer fields must be set.
>> Clear
>> all check bits that are not used.
>>> Petri Savolainen(psavol) wrote:
>>> Fail means an operation error (bad param, internal error), not e.g. error
>>> in packet header formats.
>>>> Petri Savolainen(psavol) wrote:
>>>> 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_r149940099
updated_at 2017-11-09 12:49:44
