| Message ID | 1508922007-30582-2-git-send-email-odpbot@yandex.ru |
|---|---|
| State | New |
| Headers | show |
| Series | [API-NEXT,v4,1/3] api: ipsec: rework ODP_IPSEC_SA_DISABLE into packet error | expand |
> -----Original Message----- > From: lng-odp [mailto:lng-odp-bounces@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.ereminsolenikov@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.ereminsolenikov@linaro.org> > Cc: Nikhil Agarwal <nikhil.agarwal@linaro.org> > Cc: Balasubramanian Manoharan <bala.manoharan@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). 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). 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. 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. 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. -Petri
On 26/10/17 14:55, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > >> -----Original Message----- >> From: lng-odp [mailto:lng-odp-bounces@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.ereminsolenikov@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.ereminsolenikov@linaro.org> >> Cc: Nikhil Agarwal <nikhil.agarwal@linaro.org> >> Cc: Balasubramanian Manoharan <bala.manoharan@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
> -----Original Message----- > From: Dmitry Eremin-Solenikov [mailto:dmitry.ereminsolenikov@linaro.org] > Sent: Thursday, October 26, 2017 3:45 PM > To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia.com>; > Github ODP bot <odpbot@yandex.ru>; lng-odp@lists.linaro.org > Subject: Re: [lng-odp] [PATCH API-NEXT v4 1/3] api: ipsec: rework > ODP_IPSEC_SA_DISABLE into packet error > > On 26/10/17 14:55, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > > > > >> -----Original Message----- > >> From: lng-odp [mailto:lng-odp-bounces@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.ereminsolenikov@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.ereminsolenikov@linaro.org> > >> Cc: Nikhil Agarwal <nikhil.agarwal@linaro.org> > >> Cc: Balasubramanian Manoharan <bala.manoharan@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. Maybe then it should not be called artificial packet but an error packet. A packet that has odp_packet_has_error() set (on main type level), but on closer look it would be an ipsec packet with ipsec error/warning set. Still application would need to distinguish if this packet was actually sent by the remote end (a valid packet received from the tunnel, but marked with SA disable warning) or not (was not received from the tunnel). > > > > > 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. I think this is detail is not still answered: how dummy packet event can be supported directly, but SA disable event cannot? A dummy packet is generated and recognized by SW. This could be an example of how event types are coded. Keeping ipsec status as a non-packet might not cost anything extra. odp_event_type_t odp_event_type(odp_event_t event) { pkt_desc = (void *)event; if (desc->hw_block.crypto) return ODP_EVENT_CRYPTO_COMPL; if (unlikely(pkt_desc->len == 0)) { // non-packet event, determine actual event type if (desc->tmo) return ODP_EVENT_TIMEOUT; if (desc->ipsec_sa) return ODP_EVENT_IPSEC_STATUS; } // common packets (length > 0, not from bulk crypto) return ODP_EVENT_PACKET; } > > > 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. Example above demonstrates that it may not be a problem at all. So, what is the problem and how large it is. Why additional packet with a flag is OK, but the same packet with another event type returned is not OK? -Petri
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. * * @param sa IPSEC SA to be disabled * @@ -914,6 +916,12 @@ typedef struct odp_ipsec_error_t { /** Hard lifetime expired: packets */ uint32_t hard_exp_packets : 1; + + /** + * SA is disabled, all packets are processed, it is + * safe to destroy it now. + */ + uint32_t sa_disabled : 1; }; /** All error bits @@ -927,7 +935,13 @@ typedef struct odp_ipsec_error_t { } odp_ipsec_error_t; -/** IPSEC warnings */ +/** + * IPSEC warnings + * + * For outbound SAs in ODP_IPSEC_OP_MODE_INLINE mode warnings can be reported + * only as status events. In all other cases warnings are reported as a part of + * IPsec packet result metadata. + */ typedef struct odp_ipsec_warn_t { /** IPSEC warnings */ union { @@ -1133,15 +1147,6 @@ typedef struct odp_ipsec_packet_result_t { * IPSEC status ID */ typedef enum odp_ipsec_status_id_t { - /** Response to SA disable command - * - * Following status event (odp_ipsec_status_t) fields have valid - * content, other fields must be ignored: - * - sa: The SA that was requested to be disabled - * - result: Operation result - */ - ODP_IPSEC_STATUS_SA_DISABLE = 0, - /** Warning from inline IPSEC processing * * Following status event (odp_ipsec_status_t) fields have valid @@ -1152,7 +1157,7 @@ typedef enum odp_ipsec_status_id_t { * This status event is generated only for outbound SAs in * ODP_IPSEC_OP_MODE_INLINE mode. */ - ODP_IPSEC_STATUS_WARN + ODP_IPSEC_STATUS_WARN = 0 } odp_ipsec_status_id_t; @@ -1166,13 +1171,6 @@ typedef struct odp_ipsec_status_t { /** IPSEC SA that was target of the operation */ odp_ipsec_sa_t sa; - /** Result of the operation - * - * 0: Success - * <0: Failure - */ - int result; - /** Warnings of an ODP_IPSEC_STATUS_WARN status event */ odp_ipsec_warn_t warn; @@ -1469,8 +1467,6 @@ int odp_ipsec_result(odp_ipsec_packet_result_t *result, odp_packet_t packet); * * @retval 0 On success * @retval <0 On failure - * - * @see odp_ipsec_sa_disable() */ int odp_ipsec_status(odp_ipsec_status_t *status, odp_event_t event);