Petri Savolainen(psavol) replied on github web page:
include/odp/api/spec/packet.h
line 87
@@ -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;
+
Comment:
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_r149924779
updated_at 2017-11-09 12:49:44
