On Tue, Oct 24, 2017 at 7:49 AM, Peltonen, Janne (Nokia - FI/Espoo) < [email protected]> wrote:
> Hi, > > Comments below: > > > -----Original Message----- > > From: lng-odp [mailto:[email protected]] On Behalf Of > Github ODP bot > > Sent: Tuesday, October 24, 2017 2:00 PM > > To: [email protected] > > Subject: [lng-odp] [PATCH API-NEXT v1 1/3] api: ipsec: rework > ODP_IPSEC_SA_DISABLE into > > packet error > > > > From: Dmitry Eremin-Solenikov <[email protected]> > > > > 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 <[email protected]> > > Cc: Balasubramanian Manoharan <[email protected]> > > --- > > /** 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: 1bab6bb255422c8eb061c6482397eb498fc7b1cc > > **/ > > include/odp/api/spec/ipsec.h | 38 ++++++++++++++---------------- > -------- > > 1 file changed, 14 insertions(+), 24 deletions(-) > > > > diff --git a/include/odp/api/spec/ipsec.h b/include/odp/api/spec/ipsec.h > > index 26e852fca..f550b6bac 100644 > > --- a/include/odp/api/spec/ipsec.h > > +++ b/include/odp/api/spec/ipsec.h > > @@ -842,11 +842,12 @@ odp_ipsec_sa_t odp_ipsec_sa_create(const > odp_ipsec_sa_param_t > > *param); > > * the SA and be processed successfully. > > * > > * 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 > > + * to destroy the SA. In asynchronous mode, the same is indicated by a > packet > > + * event sent to the queue specified for the SA having sa_disabled > error bit > > + * set. 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. > > Maybe it should be clarified that the packet event with sa_disabled > error bit set will be sent even when there is no traffic on the SA, > i.e. no acual packets being processed. > > Then I wonder if "packet event with sa_disabled error bit set" is too > shortened an expression that requires the reader to figure out that it > really means such ODP_EVENT_PACKET with subtype ODP_EVENT_PACKET_IPSEC > that odp_ipsec_result(odp_ipsec_packet_from_event(ev)) indicates > sa_disabled through the error bits. > > If the packet resulting from odp_ipsec_packet_from_event() must never > be used as a packet as Bill summarized in his post, then that should be > specified somewhere, maybe in the documentation of > odp_ipsec_packet_from_event(). > > event.h says this about ODP_EVENT_PACKET: > > * - ODP_EVENT_PACKET > * - Packet event (odp_packet_t) containing packet data and plenty of > * packet processing related metadata > > But that does not really apply to the "sa_disabled" packet events. > > Then event.h says this regarding event subtypes: > > * When > * subtype is known, these subtype specific functions should be preferred > over > * the event type general function (e.g. odp_packet_from_event()). > > This implies that even though odp_ipsec_packet_from_event() is "preferred", > odp_packet_from_event() is ok even for the sa_disabled ipsec packet events. > But if the resulting packet must not be used as a packet, then one needs > to always check the subtype too. If some packet events are not really > packets, the whole packet event type and subtype thing does not seem > terribly useful to me. > This was the idea behind having this as a separate event type, but that's what seems problematic for some implementations. We could introduce another subtype (ODP_EVENT_PACKET_IPSEC_DISABLE?) but that starts to get ugly. I think it's sufficient to say that the application is responsible to know that these "packets" are not to be used as actual packets but only as carriers fo the odp_ipsec_result_t metadata. The odp_packet_len() for these packets is set to zero as an additional indication so even if the application used odp_packet_from_event() that would still tell it that something is different about this packet. The zero length convention should probably be formalized in the spec for that reason. > > > > + * events) have been enqueued before it. The will be no more packets > coming > > + * from SA queue. > > The last sentence (which has a typo, btw) is not correct since more > packets can be coming through the queue, not for the SA that was > disabled but for other SAs. > > > * > > * @param sa IPSEC SA to be disabled > > * > > @@ -914,6 +915,8 @@ typedef struct odp_ipsec_error_t { > > > > /** Hard lifetime expired: packets */ > > uint32_t hard_exp_packets : 1; > > + > > + uint32_t sa_disabled : 1; > > Doxygen comment is missing. > Good catch. Looks like we have a problem with the Doxygen checker in Travis. Doxygen actually flagged this but the test didn't pick that up. > > > }; > > > > /** All error bits > > @@ -927,7 +930,12 @@ 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 can be reported > either as > > + * a part of packet result or via separate ODP status event. > > + */ > > Reporting warnings in the other cases either through packet result or > status > event is not according to Bill's summary and not consistent with the > comments > below. > > As we agreed, the only current use of status events is outbound inline soft expiration warnings. Spec should reflect that. > > > typedef struct odp_ipsec_warn_t { > > /** IPSEC warnings */ > > union { > > @@ -1133,15 +1141,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 +1151,7 @@ typedef enum odp_ipsec_status_id_t { > > * This status event is generated only for outbound SAs in > > * ODP_IPSEC_OP_MODE_INLINE mode. > > */ > > Here it says that the status event is only for the inline outbound case. > > > - ODP_IPSEC_STATUS_WARN > > + ODP_IPSEC_STATUS_WARN = 0 > > > > } odp_ipsec_status_id_t; > > > > @@ -1166,13 +1165,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 +1461,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); > > > >
