diff mbox series

[API-NEXT,v1,2/2] api: ipsec: move soft limits expiration to flags, rather than errors

Message ID 1493917206-2630-2-git-send-email-odpbot@yandex.ru
State New
Headers show
Series [API-NEXT,v1,1/2] api: ipsec: add soft limit expiration event | expand

Commit Message

Github ODP bot May 4, 2017, 5 p.m. UTC
From: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>


Soft limit expiration isn't an error per se. It does not mean, that we
received invalid or unprocessed packet. They look more like flags,
noting that soft limit on this SA was expired.

Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>

---
/** Email created from pull request 22 (lumag:ipsec-limits)
 ** https://github.com/Linaro/odp/pull/22
 ** Patch: https://github.com/Linaro/odp/pull/22.patch
 ** Base sha: 0707c974ed19c859fb92778c35a2f92bf7cd9fc6
 ** Merge commit sha: bff71bdc47fecb62fced59449c139d3ea4b44def
 **/
 include/odp/api/spec/ipsec.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Comments

Savolainen, Petri (Nokia - FI/Espoo) May 5, 2017, 8:17 a.m. UTC | #1
> -----Original Message-----

> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of

> Github ODP bot

> Sent: Thursday, May 04, 2017 8:00 PM

> To: lng-odp@lists.linaro.org

> Subject: [lng-odp] [PATCH API-NEXT v1 2/2] api: ipsec: move soft limits

> expiration to flags, rather than errors

> 

> From: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>

> 

> Soft limit expiration isn't an error per se. It does not mean, that we

> received invalid or unprocessed packet. They look more like flags,

> noting that soft limit on this SA was expired.


Advantage of keeping those among error flags, is the easy/fast check in the common case:

Only check (error.u64 == 0) to see that everything is OK vs.

checking always both (error.u64 == 0 && (status.u64 == 0 || (status.soft_exp_sec == 0 && status.soft_exp_bytes == 0 && status.soft_exp_packets == 0)))


-Petri
Peltonen, Janne (Nokia - FI/Espoo) May 5, 2017, 2:01 p.m. UTC | #2
There is perhaps some ambiguity now in the API on whether expired soft
lifetime means that a packet is not "successfully processed", which in
turn determines whether inline mode outbound packets get sent directly out.

Having the life time expiration in the error field could be interpreted
to imply that soft life time expiration in inline output is not considered
successful processing, so an application can expect getting a result event
with successfully transformed packet but with one of the soft lifetime
error bits set. That works, but I do not think we want to drop out from
inline processing mode just because of soft lifetime (/bytes/packets)
expiration.

	Janne


> -----Original Message-----

> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Savolainen, Petri

> (Nokia - FI/Espoo)

> Sent: Friday, May 05, 2017 11:18 AM

> To: lng-odp@lists.linaro.org

> Subject: Suspected SPAM - Re: [lng-odp] [PATCH API-NEXT v1 2/2] api: ipsec: move soft

> limits expiration to flags, rather than errors

> 

> 

> 

> > -----Original Message-----

> > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of

> > Github ODP bot

> > Sent: Thursday, May 04, 2017 8:00 PM

> > To: lng-odp@lists.linaro.org

> > Subject: [lng-odp] [PATCH API-NEXT v1 2/2] api: ipsec: move soft limits

> > expiration to flags, rather than errors

> >

> > From: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>

> >

> > Soft limit expiration isn't an error per se. It does not mean, that we

> > received invalid or unprocessed packet. They look more like flags,

> > noting that soft limit on this SA was expired.

> 

> Advantage of keeping those among error flags, is the easy/fast check in the common case:

> 

> Only check (error.u64 == 0) to see that everything is OK vs.

> 

> checking always both (error.u64 == 0 && (status.u64 == 0 || (status.soft_exp_sec == 0 &&

> status.soft_exp_bytes == 0 && status.soft_exp_packets == 0)))

> 

> 

> -Petri
Bill Fischofer May 5, 2017, 2:17 p.m. UTC | #3
On Fri, May 5, 2017 at 9:01 AM, Peltonen, Janne (Nokia - FI/Espoo) <
janne.peltonen@nokia.com> wrote:

> There is perhaps some ambiguity now in the API on whether expired soft

> lifetime means that a packet is not "successfully processed", which in

> turn determines whether inline mode outbound packets get sent directly out.

>

> Having the life time expiration in the error field could be interpreted

> to imply that soft life time expiration in inline output is not considered

> successful processing, so an application can expect getting a result event

> with successfully transformed packet but with one of the soft lifetime

> error bits set. That works, but I do not think we want to drop out from

> inline processing mode just because of soft lifetime (/bytes/packets)

>


Agree. which is why soft limits should be treated as separate events. These
are essentially alerts to the control plane that it's time to start
rekeying this SA as the current keys are nearing the end of their useful
life. So in that case it's like the low fuel indicator on a car. That
doesn't mean the car stops, just means that you need to consider filling up
in the near future.

Since as previously noted time based limits need to be treated as events
anyway since they are not associated with any packet, it's simpler and more
uniform to treat all such limit crossings the same. Adding side-information
to the processing results for a given packet means that the application
would need to check every packet for these rather than simply having the
appropriate event handler waiting to receive these relatively rare
notifications.


> expiration.

>

>         Janne

>

>

> > -----Original Message-----

> > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of

> Savolainen, Petri

> > (Nokia - FI/Espoo)

> > Sent: Friday, May 05, 2017 11:18 AM

> > To: lng-odp@lists.linaro.org

> > Subject: Suspected SPAM - Re: [lng-odp] [PATCH API-NEXT v1 2/2] api:

> ipsec: move soft

> > limits expiration to flags, rather than errors

> >

> >

> >

> > > -----Original Message-----

> > > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of

> > > Github ODP bot

> > > Sent: Thursday, May 04, 2017 8:00 PM

> > > To: lng-odp@lists.linaro.org

> > > Subject: [lng-odp] [PATCH API-NEXT v1 2/2] api: ipsec: move soft limits

> > > expiration to flags, rather than errors

> > >

> > > From: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>

> > >

> > > Soft limit expiration isn't an error per se. It does not mean, that we

> > > received invalid or unprocessed packet. They look more like flags,

> > > noting that soft limit on this SA was expired.

> >

> > Advantage of keeping those among error flags, is the easy/fast check in

> the common case:

> >

> > Only check (error.u64 == 0) to see that everything is OK vs.

> >

> > checking always both (error.u64 == 0 && (status.u64 == 0 ||

> (status.soft_exp_sec == 0 &&

> > status.soft_exp_bytes == 0 && status.soft_exp_packets == 0)))

> >

> >

> > -Petri

>

>
Bill Fischofer May 5, 2017, 4:02 p.m. UTC | #4
On Fri, May 5, 2017 at 3:17 AM, Savolainen, Petri (Nokia - FI/Espoo) <
petri.savolainen@nokia.com> wrote:

>

>

> > -----Original Message-----

> > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of

> > Github ODP bot

> > Sent: Thursday, May 04, 2017 8:00 PM

> > To: lng-odp@lists.linaro.org

> > Subject: [lng-odp] [PATCH API-NEXT v1 2/2] api: ipsec: move soft limits

> > expiration to flags, rather than errors

> >

> > From: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>

> >

> > Soft limit expiration isn't an error per se. It does not mean, that we

> > received invalid or unprocessed packet. They look more like flags,

> > noting that soft limit on this SA was expired.

>

> Advantage of keeping those among error flags, is the easy/fast check in

> the common case:

>

> Only check (error.u64 == 0) to see that everything is OK vs.

>

> checking always both (error.u64 == 0 && (status.u64 == 0 ||

> (status.soft_exp_sec == 0 && status.soft_exp_bytes == 0 &&

> status.soft_exp_packets == 0)))

>


Agree on keeping error bits together for check simplicity, but as discussed
on other threads status events should be their own events, not piggybacked
onto operation results since these are essentially control signals rather
than things that worker threads would process. The point is that a worker
receiving this notification can only pass it along to someone else since
workers aren't (or shouldn't) be involved in rekeying. So better to have a
separate control thread that receives such notifications directly.


>

>

> -Petri

>

>
Dmitry Eremin-Solenikov May 5, 2017, 9:24 p.m. UTC | #5
On 05.05.2017 11:17, 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: Thursday, May 04, 2017 8:00 PM

>> To: lng-odp@lists.linaro.org

>> Subject: [lng-odp] [PATCH API-NEXT v1 2/2] api: ipsec: move soft limits

>> expiration to flags, rather than errors

>>

>> From: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>

>>

>> Soft limit expiration isn't an error per se. It does not mean, that we

>> received invalid or unprocessed packet. They look more like flags,

>> noting that soft limit on this SA was expired.

> 

> Advantage of keeping those among error flags, is the easy/fast check in the common case:

> 

> Only check (error.u64 == 0) to see that everything is OK vs.


Well, that is exactly my point for moving soft_exp out of error
bitfield. Soft expiration is not an error. The packet was processed
successfully. In case of outbound inline processing, it was even sent to
pktio.

I have following code scenarios for the application (leaving alone time
based expiration, which should be rethought, especially wrt. SYNC
processing):

- SYNC or ASYNC: application checks status.flag.soft_exp_* to check if
it needs to rekey. Additional care should be applied so that rekeying is
not started multiple times for the same SA, because further packet
status will also contains soft_exp_* flag set on.

- ASYNC or INLINE: application receives IPSEC_STATUS event for this SA
queue and then starts rekeying based on that event.

I should probably note that there is no direct need to introduce
IPSEC_STATUS events for hard timeouts, since they will end up as errors.

-- 
With best wishes
Dmitry
Bill Fischofer May 5, 2017, 9:29 p.m. UTC | #6
On Fri, May 5, 2017 at 4:24 PM, Dmitry Eremin-Solenikov
<dmitry.ereminsolenikov@linaro.org> wrote:
> On 05.05.2017 11:17, 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: Thursday, May 04, 2017 8:00 PM

>>> To: lng-odp@lists.linaro.org

>>> Subject: [lng-odp] [PATCH API-NEXT v1 2/2] api: ipsec: move soft limits

>>> expiration to flags, rather than errors

>>>

>>> From: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>

>>>

>>> Soft limit expiration isn't an error per se. It does not mean, that we

>>> received invalid or unprocessed packet. They look more like flags,

>>> noting that soft limit on this SA was expired.

>>

>> Advantage of keeping those among error flags, is the easy/fast check in the common case:

>>

>> Only check (error.u64 == 0) to see that everything is OK vs.

>

> Well, that is exactly my point for moving soft_exp out of error

> bitfield. Soft expiration is not an error. The packet was processed

> successfully. In case of outbound inline processing, it was even sent to

> pktio.

>

> I have following code scenarios for the application (leaving alone time

> based expiration, which should be rethought, especially wrt. SYNC

> processing):

>

> - SYNC or ASYNC: application checks status.flag.soft_exp_* to check if

> it needs to rekey. Additional care should be applied so that rekeying is

> not started multiple times for the same SA, because further packet

> status will also contains soft_exp_* flag set on.

>

> - ASYNC or INLINE: application receives IPSEC_STATUS event for this SA

> queue and then starts rekeying based on that event.

>

> I should probably note that there is no direct need to introduce

> IPSEC_STATUS events for hard timeouts, since they will end up as errors.


Hard and soft timeouts have the same problem in that they are
independent of any packet. So if no packets are being processed you
might never notice that a hard timeout has passed. Agree we need to
discuss this next week, but treating all such expirations uniformly as
async events seems consistent and straightforward.

>

> --

> With best wishes

> Dmitry
Dmitry Eremin-Solenikov May 5, 2017, 9:33 p.m. UTC | #7
On 06.05.2017 00:29, Bill Fischofer wrote:
> On Fri, May 5, 2017 at 4:24 PM, Dmitry Eremin-Solenikov

> <dmitry.ereminsolenikov@linaro.org> wrote:

>> On 05.05.2017 11:17, 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: Thursday, May 04, 2017 8:00 PM

>>>> To: lng-odp@lists.linaro.org

>>>> Subject: [lng-odp] [PATCH API-NEXT v1 2/2] api: ipsec: move soft limits

>>>> expiration to flags, rather than errors

>>>>

>>>> From: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>

>>>>

>>>> Soft limit expiration isn't an error per se. It does not mean, that we

>>>> received invalid or unprocessed packet. They look more like flags,

>>>> noting that soft limit on this SA was expired.

>>>

>>> Advantage of keeping those among error flags, is the easy/fast check in the common case:

>>>

>>> Only check (error.u64 == 0) to see that everything is OK vs.

>>

>> Well, that is exactly my point for moving soft_exp out of error

>> bitfield. Soft expiration is not an error. The packet was processed

>> successfully. In case of outbound inline processing, it was even sent to

>> pktio.

>>

>> I have following code scenarios for the application (leaving alone time

>> based expiration, which should be rethought, especially wrt. SYNC

>> processing):

>>

>> - SYNC or ASYNC: application checks status.flag.soft_exp_* to check if

>> it needs to rekey. Additional care should be applied so that rekeying is

>> not started multiple times for the same SA, because further packet

>> status will also contains soft_exp_* flag set on.

>>

>> - ASYNC or INLINE: application receives IPSEC_STATUS event for this SA

>> queue and then starts rekeying based on that event.

>>

>> I should probably note that there is no direct need to introduce

>> IPSEC_STATUS events for hard timeouts, since they will end up as errors.

> 

> Hard and soft timeouts have the same problem in that they are

> independent of any packet. So if no packets are being processed you

> might never notice that a hard timeout has passed. Agree we need to

> discuss this next week, but treating all such expirations uniformly as

> async events seems consistent and straightforward.


This is for bytes and packets expiry, as stated above.

-- 
With best wishes
Dmitry
diff mbox series

Patch

diff --git a/include/odp/api/spec/ipsec.h b/include/odp/api/spec/ipsec.h
index e83494d..384c43d 100644
--- a/include/odp/api/spec/ipsec.h
+++ b/include/odp/api/spec/ipsec.h
@@ -860,15 +860,6 @@  typedef struct odp_ipsec_op_status_t {
 			/** Packet does not fit into the given MTU size */
 			uint32_t mtu              : 1;
 
-			/** Soft lifetime expired: seconds */
-			uint32_t soft_exp_sec     : 1;
-
-			/** Soft lifetime expired: bytes */
-			uint32_t soft_exp_bytes   : 1;
-
-			/** Soft lifetime expired: packets */
-			uint32_t soft_exp_packets : 1;
-
 			/** Hard lifetime expired: seconds */
 			uint32_t hard_exp_sec     : 1;
 
@@ -894,6 +885,15 @@  typedef struct odp_ipsec_op_status_t {
 	union {
 		/** Status flags */
 		struct {
+			/** Soft lifetime expired: seconds */
+			uint32_t soft_exp_sec     : 1;
+
+			/** Soft lifetime expired: bytes */
+			uint32_t soft_exp_bytes   : 1;
+
+			/** Soft lifetime expired: packets */
+			uint32_t soft_exp_packets : 1;
+
 			/** Packet was processed in inline mode */
 			uint32_t inline_mode      : 1;