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:
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_r149945169
updated_at 2017-11-09 12:49:44
