On 04.04.2017 10:15, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > >> -----Original Message----- >> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of >> Dmitry Eremin-Solenikov >> Sent: Monday, April 03, 2017 10:48 PM >> To: lng-odp-forward <lng-odp@lists.linaro.org> >> Subject: [lng-odp] odp_ipsec_result behaviour >> >> Hello, >> >> I was hunting last issues in my IPsec patchset. And suddently I was hit >> by the fact that I do not fully understand semantics around >> odp_ipsec_result() function. So here comes several questions: >> >> 1) Is is correct, that I have to free the event manually even after the >> successful return from the function? > > The behavior is the same as with crypto completion (which is not well > documented). Application needs to free result event before it uses packets, > since typically the result event is actually a packet event. In that case, a > free operation would just change the event type of that event (from result to > packet). This must be done before any packet API calls are done since those > may check the event type (which must be packet at that time).
This actually can easily lead to memory leaks. _free function should determine if it was called because the event will be processed further as the packet or because it should actually free the event and underlying memory. If we'd like to follow this idea, we should explicitly disallow calling odp_event_free after successful odp_ipsec_result return. See: - odp_event_free() would always free underlying memory - odp_ipsec_result() would change buffer type from IPSEC_RESULT to PACKET, extract ipsec_op_result, etc. After this operation original event is unsuitable for further operations. >> 2) What is the expected behaviour of the function if it is called with >> result.num_pkt being too small? Should it output packets at this case? >> Is it fine to output only some of the fragments from the single input >> packet? (not that I support fragmentation at this moment, but still) >> If it is called again (with larger num_pkt), should it re-output the >> same packets, or should it continue from where it stopped? >> >> The last question being the most important one, I think. > > Currently it is stateless. So, application must call it with large enough > array to succeed. The reason is the same as above. If the result event is > actually one of the packets, application cannot process/free/forward the > packet event before all result data is extracted from it. Ugh. An application does not have a way to determine the amount of memory it needs (well, unless it depends on 1:1 event <-> packet correspondence as ipsec_offload does). > I agree that statefull function would be nicer to use in general and we may > end up changing this (and crypto completion API). We just need to make sure > that a packet can be used as the result/completion. Well, I see nothing particularly hard with handling packet as a result even with stateful implementation. Especially if we move type changing to ipsec_result() and do not require odp_event_free() call on fully processed events. > One option is to specify that result/compl is a packet and provide one way > conversion function from result/compl to packet. Before changing we should > also consider completion cases where the data may be either packet or just a > memory address. E.g. compression could have use cases for both packets > (compress data in the packet) and flat memory areas (compress my data base > before I save it to the disc) ==> compl event is not a packet. I do not think that the application should know about exact result/completion implementation. The abstraction is already good enough. -- With best wishes Dmitry
