| Message ID | 1508842806-17934-2-git-send-email-odpbot@yandex.ru |
|---|---|
| State | New |
| Headers | show |
| Series | [API-NEXT,v1,1/3] api: ipsec: rework ODP_IPSEC_SA_DISABLE into packet error | expand |
Hi, Comments below: > -----Original Message----- > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Github ODP bot > Sent: Tuesday, October 24, 2017 2:00 PM > To: lng-odp@lists.linaro.org > Subject: [lng-odp] [PATCH API-NEXT v1 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: 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. > + * 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. > }; > > /** 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. > 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); >
On Tue, Oct 24, 2017 at 7:49 AM, Peltonen, Janne (Nokia - FI/Espoo) < janne.peltonen@nokia.com> wrote: > Hi, > > Comments below: > > > -----Original Message----- > > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of > Github ODP bot > > Sent: Tuesday, October 24, 2017 2:00 PM > > To: lng-odp@lists.linaro.org > > Subject: [lng-odp] [PATCH API-NEXT v1 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: 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); > > > >
Hi, On 24/10/17 15:49, Peltonen, Janne (Nokia - FI/Espoo) wrote: Thank for the review! >> @@ -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. Hmm, true. Changed to stricter specification. -- With best wishes Dmitry
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. + * events) have been enqueued before it. The will be no more packets coming + * from SA queue. * * @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; }; /** 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. + */ 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. */ - 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);