Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
include/odp/api/spec/packet.h
line 129
@@ -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.
+ *
+ * @param pkt Packet handle
+ * @param offset Byte offset into the packet
+ * @param param Parse parameters. Proto and layer fields must be set. Clear
+ * all check bits that are not used.
+ *
+ * @retval 0 on success
+ * @retval <0 on failure
+ */
+int odp_packet_parse(odp_packet_t pkt, uint32_t offset,
+ const odp_packet_parse_param_t *param);
+
+/**
+ * Parse multiple packets
+ *
+ * Otherwise like odp_packet_parse(), but parses multiple packets. Packets may
+ * have unique offsets, but must start with the same protocol. Also, packets
are
+ * parsed up to the same protocol layer.
+ *
+ * @param pkt Packet handle array
+ * @param offset Byte offsets into the packets
+ * @param num Number of packets and offsets
+ * @param param Parse parameters. Proto and layer fields must be set. Clear
+ * all check bits that are not used.
+ *
+ * @retval 0 on success
+ * @retval <0 on failure
+ */
+int odp_packet_parse_multi(const odp_packet_t pkt[], const uint32_t offset[],
+ int num, const odp_packet_parse_param_t *param);
+
Comment:
Parsing can fail because the packet is malformed in some way. So parsing is
also an important integrity check.
> bogdanPricope wrote
> Then: "When operation fails, metadata of all protocol layers remains
> unchanged." ?
>> bogdanPricope wrote
>> 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_r149954064
updated_at 2017-11-09 12:59:36
