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.
+ * When operation fails, metadata of all protocol layers have undefined values.
Comment:
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_r149916975
updated_at 2017-11-09 12:49:44
