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