On 26/10/17 14:55, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > >> -----Original Message----- >> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of >> Github ODP bot >> Sent: Wednesday, October 25, 2017 12:00 PM >> To: lng-odp@lists.linaro.org >> Subject: [lng-odp] [PATCH API-NEXT v4 1/3] api: ipsec: rework >> ODP_IPSEC_SA_DISABLE into packet error >> >> From: Dmitry Eremin-Solenikov <dmitry.ereminsoleni...@linaro.org> >> >> According to the discussion on mailing list, most of implementations >> will not be able to support odp_ipsec_sa_disable() status event >> directly. Instead they will submit a dummy packet to that SA. Then >> after receiving this packet after odp_ipsec_result() will detect this >> packet and will report it as a packet with >> odp_ipsec_error_t->sa_disabled bit set. >> >> Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsoleni...@linaro.org> >> Cc: Nikhil Agarwal <nikhil.agar...@linaro.org> >> Cc: Balasubramanian Manoharan <bala.manoha...@linaro.org> >> --- >> /** Email created from pull request 256 (lumag:ipsec_sa_disable_v2) >> ** https://github.com/Linaro/odp/pull/256 >> ** Patch: https://github.com/Linaro/odp/pull/256.patch >> ** Base sha: 825f75ed8644ef57c5648961e7982daf13cd9375 >> ** Merge commit sha: ba520d0a3f4c46777c7aedca029e9979a89c69e7 >> **/ >> include/odp/api/spec/ipsec.h | 44 ++++++++++++++++++++------------------- >> ----- >> 1 file changed, 20 insertions(+), 24 deletions(-) >> >> diff --git a/include/odp/api/spec/ipsec.h b/include/odp/api/spec/ipsec.h >> index 26e852fca..b9ad282ce 100644 >> --- a/include/odp/api/spec/ipsec.h >> +++ b/include/odp/api/spec/ipsec.h >> @@ -843,10 +843,12 @@ odp_ipsec_sa_t odp_ipsec_sa_create(const >> odp_ipsec_sa_param_t *param); >> * >> * When in synchronous operation mode, the call will return when it's >> possible >> * to destroy the SA. In asynchronous mode, the same is indicated by an >> - * ODP_EVENT_IPSEC_STATUS event sent to the queue specified for the SA. >> The >> - * status event is guaranteed to be the last event for the SA, i.e. all >> - * in-progress operations have completed and resulting events (including >> status >> - * events) have been enqueued before it. >> + * artificial packet event sent to the queue specified for the SA having >> + * sa_disabled error bit set in the odp_ipsec_packet_result_t returned by >> + * odp_ipsec_result(). The packet is guaranteed to be the last event for >> + * the SA, i.e. all in-progress operations have completed and resulting >> events >> + * (including status events) have been enqueued before it. No packets >> will come >> + * from SA after this one. > > This still lacks the definition of what is the difference between an > artificial packet vs a normal packet. We could define it e.g. by saying that > length is always zero and application must not do anything else with it than > free it. But then, why it's a packet anyway? Why it would not then be e.g. a > ipsec status event (which is not a packet, but could be implemented as an > artificial packet).
No difference from my POV. Processing path for all packets coming from IPsec: - Error packet. Process error flags, drop the packet. - Not an error. Process warning flags, forward the packet. > > The idea of event type vs sub-event type is that: > 1) all events of the same base type (e.g. packet) contain the same metadata > and can be processed the same way. For example, someone may write a generic > packet statistics (application) module, and plug that before and after an > ipsec termination (application) module, without need to know that packets > before ipsec module carry extra ipsec metadata (from inline inbound ipsec). As long as they don't carry error flag. odp_packet_has_error(), etc. BTW: I should check that my implementation sets that flag. > 2) sub-type adds metadata and other properties to the base type > > An artificial packet would not fit in either category, it would be a > non-packet. I somewhat agree with you here. I would prefer separate event for SA_DISABLE. However some (most?) hardware ATM will not be able to support such an event directly. And an application will need separate processing for error packets anyway. > Usually, application receives multiple event types anyway: e.g. packets and > timeouts. So, for application it would not be an issue to have one additional > switch-case for IPSEC status events. API integrity is more important than > (potential) save of couple if-else branches. It is not a problem of application, but rather an implementation problem. > So, I think this solution is not complete yet and I don't see how it would in > the end make a real difference to the current status event API. -- With best wishes Dmitry
