> -----Original Message-----
> From: Dmitry Eremin-Solenikov [mailto:dmitry.ereminsoleni...@linaro.org]
> Sent: Thursday, October 26, 2017 3:45 PM
> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>;
> Github ODP bot <odp...@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-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.
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
