| Message ID | 7d08b42f4c1480caa855776d92331fe9beed001d.1571033544.git.lucien.xin@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | sctp: update from rfc7829 | expand |
I've found v3 :-) But it isn't that much better than v2. From: Xin Long > Sent: 14 October 2019 07:15 > SCTP Quick failover draft section 5.1, point 5 has been removed > from rfc7829. Instead, "the sender SHOULD (i) notify the Upper > Layer Protocol (ULP) about this state transition", as said in > section 3.2, point 8. > > So this patch is to add SCTP_ADDR_POTENTIALLY_FAILED, defined > in section 7.1, "which is reported if the affected address > becomes PF". Also remove transport cwnd's update when moving > from PF back to ACTIVE , which is no longer in rfc7829 either. > > v1->v2: > - no change > v2->v3: > - define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > --- > include/uapi/linux/sctp.h | 2 ++ > net/sctp/associola.c | 17 ++++------------- > 2 files changed, 6 insertions(+), 13 deletions(-) > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > index 6bce7f9..f4ab7bb 100644 > --- a/include/uapi/linux/sctp.h > +++ b/include/uapi/linux/sctp.h > @@ -410,6 +410,8 @@ enum sctp_spc_state { > SCTP_ADDR_ADDED, > SCTP_ADDR_MADE_PRIM, > SCTP_ADDR_CONFIRMED, > + SCTP_ADDR_POTENTIALLY_FAILED, > +#define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED > }; > > > diff --git a/net/sctp/associola.c b/net/sctp/associola.c > index 1ba893b..4f9efba 100644 > --- a/net/sctp/associola.c > +++ b/net/sctp/associola.c > @@ -801,14 +801,6 @@ void sctp_assoc_control_transport(struct sctp_association *asoc, > spc_state = SCTP_ADDR_CONFIRMED; > else > spc_state = SCTP_ADDR_AVAILABLE; > - /* Don't inform ULP about transition from PF to > - * active state and set cwnd to 1 MTU, see SCTP > - * Quick failover draft section 5.1, point 5 > - */ > - if (transport->state == SCTP_PF) { > - ulp_notify = false; > - transport->cwnd = asoc->pathmtu; > - } This is wrong. If the old state is PF and the application hasn't exposed PF the event should be ignored. > transport->state = SCTP_ACTIVE; > break; > > @@ -817,19 +809,18 @@ void sctp_assoc_control_transport(struct sctp_association *asoc, > * to inactive state. Also, release the cached route since > * there may be a better route next time. > */ > - if (transport->state != SCTP_UNCONFIRMED) > + if (transport->state != SCTP_UNCONFIRMED) { > transport->state = SCTP_INACTIVE; > - else { > + spc_state = SCTP_ADDR_UNREACHABLE; > + } else { > sctp_transport_dst_release(transport); > ulp_notify = false; > } > - > - spc_state = SCTP_ADDR_UNREACHABLE; > break; > > case SCTP_TRANSPORT_PF: > transport->state = SCTP_PF; > - ulp_notify = false; Again the event should be supressed if PF isn't exposed. > + spc_state = SCTP_ADDR_POTENTIALLY_FAILED; > break; > > default: > -- > 2.1.0 I also haven't spotted where the test that the application has actually enabled state transition events is in the code. I'd have thought it would be anything is built and allocated. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Fri, Oct 18, 2019 at 11:56 PM David Laight <David.Laight@aculab.com> wrote: > > I've found v3 :-) ah okay. sorry. > But it isn't that much better than v2. > > From: Xin Long > > Sent: 14 October 2019 07:15 > > SCTP Quick failover draft section 5.1, point 5 has been removed > > from rfc7829. Instead, "the sender SHOULD (i) notify the Upper > > Layer Protocol (ULP) about this state transition", as said in > > section 3.2, point 8. > > > > So this patch is to add SCTP_ADDR_POTENTIALLY_FAILED, defined > > in section 7.1, "which is reported if the affected address > > becomes PF". Also remove transport cwnd's update when moving > > from PF back to ACTIVE , which is no longer in rfc7829 either. > > > > v1->v2: > > - no change > > v2->v3: > > - define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > --- > > include/uapi/linux/sctp.h | 2 ++ > > net/sctp/associola.c | 17 ++++------------- > > 2 files changed, 6 insertions(+), 13 deletions(-) > > > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > > index 6bce7f9..f4ab7bb 100644 > > --- a/include/uapi/linux/sctp.h > > +++ b/include/uapi/linux/sctp.h > > @@ -410,6 +410,8 @@ enum sctp_spc_state { > > SCTP_ADDR_ADDED, > > SCTP_ADDR_MADE_PRIM, > > SCTP_ADDR_CONFIRMED, > > + SCTP_ADDR_POTENTIALLY_FAILED, > > +#define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED > > }; > > > > > > diff --git a/net/sctp/associola.c b/net/sctp/associola.c > > index 1ba893b..4f9efba 100644 > > --- a/net/sctp/associola.c > > +++ b/net/sctp/associola.c > > @@ -801,14 +801,6 @@ void sctp_assoc_control_transport(struct sctp_association *asoc, > > spc_state = SCTP_ADDR_CONFIRMED; > > else > > spc_state = SCTP_ADDR_AVAILABLE; > > - /* Don't inform ULP about transition from PF to > > - * active state and set cwnd to 1 MTU, see SCTP > > - * Quick failover draft section 5.1, point 5 > > - */ > > - if (transport->state == SCTP_PF) { > > - ulp_notify = false; > > - transport->cwnd = asoc->pathmtu; > > - } > > This is wrong. > If the old state is PF and the application hasn't exposed PF the event should be > ignored. yeps, in Patch 2/5: + if (transport->state == SCTP_PF && + asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE) + ulp_notify = false; + else if (transport->state == SCTP_UNCONFIRMED && + error == SCTP_HEARTBEAT_SUCCESS) spc_state = SCTP_ADDR_CONFIRMED; else spc_state = SCTP_ADDR_AVAILABLE; > > > transport->state = SCTP_ACTIVE; > > break; > > > > @@ -817,19 +809,18 @@ void sctp_assoc_control_transport(struct sctp_association *asoc, > > * to inactive state. Also, release the cached route since > > * there may be a better route next time. > > */ > > - if (transport->state != SCTP_UNCONFIRMED) > > + if (transport->state != SCTP_UNCONFIRMED) { > > transport->state = SCTP_INACTIVE; > > - else { > > + spc_state = SCTP_ADDR_UNREACHABLE; > > + } else { > > sctp_transport_dst_release(transport); > > ulp_notify = false; > > } > > - > > - spc_state = SCTP_ADDR_UNREACHABLE; > > break; > > > > case SCTP_TRANSPORT_PF: > > transport->state = SCTP_PF; > > - ulp_notify = false; > > Again the event should be supressed if PF isn't exposed. it will be suppressed after Patch 2/5: + if (asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE) + ulp_notify = false; + else + spc_state = SCTP_ADDR_POTENTIALLY_FAILED; break; > > > + spc_state = SCTP_ADDR_POTENTIALLY_FAILED; > > break; > > > > default: > > -- > > 2.1.0 > > I also haven't spotted where the test that the application has actually enabled > state transition events is in the code. all events will be created, but dropped in sctp_ulpq_tail_event() when trying to deliver up: /* Check if the user wishes to receive this event. */ if (!sctp_ulpevent_is_enabled(event, ulpq->asoc->subscribe)) goto out_free; > I'd have thought it would be anything is built and allocated. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) >
Hi, David L. I will repost if you don't have any other dissent. Thanks for your nice review. On Sat, Oct 19, 2019 at 4:55 PM Xin Long <lucien.xin@gmail.com> wrote: > > On Fri, Oct 18, 2019 at 11:56 PM David Laight <David.Laight@aculab.com> wrote: > > > > I've found v3 :-) > ah okay. sorry. > > > But it isn't that much better than v2. > > > > From: Xin Long > > > Sent: 14 October 2019 07:15 > > > SCTP Quick failover draft section 5.1, point 5 has been removed > > > from rfc7829. Instead, "the sender SHOULD (i) notify the Upper > > > Layer Protocol (ULP) about this state transition", as said in > > > section 3.2, point 8. > > > > > > So this patch is to add SCTP_ADDR_POTENTIALLY_FAILED, defined > > > in section 7.1, "which is reported if the affected address > > > becomes PF". Also remove transport cwnd's update when moving > > > from PF back to ACTIVE , which is no longer in rfc7829 either. > > > > > > v1->v2: > > > - no change > > > v2->v3: > > > - define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED > > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > > --- > > > include/uapi/linux/sctp.h | 2 ++ > > > net/sctp/associola.c | 17 ++++------------- > > > 2 files changed, 6 insertions(+), 13 deletions(-) > > > > > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > > > index 6bce7f9..f4ab7bb 100644 > > > --- a/include/uapi/linux/sctp.h > > > +++ b/include/uapi/linux/sctp.h > > > @@ -410,6 +410,8 @@ enum sctp_spc_state { > > > SCTP_ADDR_ADDED, > > > SCTP_ADDR_MADE_PRIM, > > > SCTP_ADDR_CONFIRMED, > > > + SCTP_ADDR_POTENTIALLY_FAILED, > > > +#define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED > > > }; > > > > > > > > > diff --git a/net/sctp/associola.c b/net/sctp/associola.c > > > index 1ba893b..4f9efba 100644 > > > --- a/net/sctp/associola.c > > > +++ b/net/sctp/associola.c > > > @@ -801,14 +801,6 @@ void sctp_assoc_control_transport(struct sctp_association *asoc, > > > spc_state = SCTP_ADDR_CONFIRMED; > > > else > > > spc_state = SCTP_ADDR_AVAILABLE; > > > - /* Don't inform ULP about transition from PF to > > > - * active state and set cwnd to 1 MTU, see SCTP > > > - * Quick failover draft section 5.1, point 5 > > > - */ > > > - if (transport->state == SCTP_PF) { > > > - ulp_notify = false; > > > - transport->cwnd = asoc->pathmtu; > > > - } > > > > This is wrong. > > If the old state is PF and the application hasn't exposed PF the event should be > > ignored. > yeps, in Patch 2/5: > + if (transport->state == SCTP_PF && > + asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE) > + ulp_notify = false; > + else if (transport->state == SCTP_UNCONFIRMED && > + error == SCTP_HEARTBEAT_SUCCESS) > spc_state = SCTP_ADDR_CONFIRMED; > else > spc_state = SCTP_ADDR_AVAILABLE; > > > > > > transport->state = SCTP_ACTIVE; > > > break; > > > > > > @@ -817,19 +809,18 @@ void sctp_assoc_control_transport(struct sctp_association *asoc, > > > * to inactive state. Also, release the cached route since > > > * there may be a better route next time. > > > */ > > > - if (transport->state != SCTP_UNCONFIRMED) > > > + if (transport->state != SCTP_UNCONFIRMED) { > > > transport->state = SCTP_INACTIVE; > > > - else { > > > + spc_state = SCTP_ADDR_UNREACHABLE; > > > + } else { > > > sctp_transport_dst_release(transport); > > > ulp_notify = false; > > > } > > > - > > > - spc_state = SCTP_ADDR_UNREACHABLE; > > > break; > > > > > > case SCTP_TRANSPORT_PF: > > > transport->state = SCTP_PF; > > > - ulp_notify = false; > > > > Again the event should be supressed if PF isn't exposed. > it will be suppressed after Patch 2/5: > + if (asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE) > + ulp_notify = false; > + else > + spc_state = SCTP_ADDR_POTENTIALLY_FAILED; > break; > > > > > > + spc_state = SCTP_ADDR_POTENTIALLY_FAILED; > > > break; > > > > > > default: > > > -- > > > 2.1.0 > > > > I also haven't spotted where the test that the application has actually enabled > > state transition events is in the code. > all events will be created, but dropped in sctp_ulpq_tail_event() when trying > to deliver up: > > /* Check if the user wishes to receive this event. */ > if (!sctp_ulpevent_is_enabled(event, ulpq->asoc->subscribe)) > goto out_free; > > > I'd have thought it would be anything is built and allocated. > > > > David > > > > - > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > > Registration No: 1397386 (Wales) > >
Hi, Sorry for the long delay on this review. On Mon, Oct 14, 2019 at 02:14:44PM +0800, Xin Long wrote: > SCTP Quick failover draft section 5.1, point 5 has been removed > from rfc7829. Instead, "the sender SHOULD (i) notify the Upper > Layer Protocol (ULP) about this state transition", as said in > section 3.2, point 8. > > So this patch is to add SCTP_ADDR_POTENTIALLY_FAILED, defined > in section 7.1, "which is reported if the affected address > becomes PF". Also remove transport cwnd's update when moving > from PF back to ACTIVE , which is no longer in rfc7829 either. > > v1->v2: > - no change > v2->v3: > - define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > --- > include/uapi/linux/sctp.h | 2 ++ > net/sctp/associola.c | 17 ++++------------- > 2 files changed, 6 insertions(+), 13 deletions(-) > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > index 6bce7f9..f4ab7bb 100644 > --- a/include/uapi/linux/sctp.h > +++ b/include/uapi/linux/sctp.h > @@ -410,6 +410,8 @@ enum sctp_spc_state { > SCTP_ADDR_ADDED, > SCTP_ADDR_MADE_PRIM, > SCTP_ADDR_CONFIRMED, > + SCTP_ADDR_POTENTIALLY_FAILED, > +#define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED > }; > > > diff --git a/net/sctp/associola.c b/net/sctp/associola.c > index 1ba893b..4f9efba 100644 > --- a/net/sctp/associola.c > +++ b/net/sctp/associola.c > @@ -801,14 +801,6 @@ void sctp_assoc_control_transport(struct sctp_association *asoc, While at here, dealing with spc_state, please seize the moment and initialize it to the enum instead: @@ -787,7 +787,7 @@ void sctp_assoc_control_transport(struct sctp_association *asoc, sctp_sn_error_t error) { bool ulp_notify = true; - int spc_state = 0; + int spc_state = SCTP_ADDR_AVAILABLE; > spc_state = SCTP_ADDR_CONFIRMED; > else > spc_state = SCTP_ADDR_AVAILABLE; This else could be removed (equals to initial value). > - /* Don't inform ULP about transition from PF to > - * active state and set cwnd to 1 MTU, see SCTP > - * Quick failover draft section 5.1, point 5 > - */ > - if (transport->state == SCTP_PF) { > - ulp_notify = false; > - transport->cwnd = asoc->pathmtu; > - } > transport->state = SCTP_ACTIVE; > break; > > @@ -817,19 +809,18 @@ void sctp_assoc_control_transport(struct sctp_association *asoc, > * to inactive state. Also, release the cached route since > * there may be a better route next time. > */ > - if (transport->state != SCTP_UNCONFIRMED) > + if (transport->state != SCTP_UNCONFIRMED) { > transport->state = SCTP_INACTIVE; > - else { > + spc_state = SCTP_ADDR_UNREACHABLE; > + } else { > sctp_transport_dst_release(transport); > ulp_notify = false; > } > - > - spc_state = SCTP_ADDR_UNREACHABLE; > break; > > case SCTP_TRANSPORT_PF: > transport->state = SCTP_PF; > - ulp_notify = false; > + spc_state = SCTP_ADDR_POTENTIALLY_FAILED; > break; > > default: > -- > 2.1.0 >
On Sat, Oct 19, 2019 at 04:55:01PM +0800, Xin Long wrote: > > > @@ -801,14 +801,6 @@ void sctp_assoc_control_transport(struct sctp_association *asoc, > > > spc_state = SCTP_ADDR_CONFIRMED; > > > else > > > spc_state = SCTP_ADDR_AVAILABLE; > > > - /* Don't inform ULP about transition from PF to > > > - * active state and set cwnd to 1 MTU, see SCTP > > > - * Quick failover draft section 5.1, point 5 > > > - */ > > > - if (transport->state == SCTP_PF) { > > > - ulp_notify = false; > > > - transport->cwnd = asoc->pathmtu; > > > - } > > > > This is wrong. > > If the old state is PF and the application hasn't exposed PF the event should be > > ignored. > yeps, in Patch 2/5: > + if (transport->state == SCTP_PF && > + asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE) > + ulp_notify = false; > + else if (transport->state == SCTP_UNCONFIRMED && > + error == SCTP_HEARTBEAT_SUCCESS) > spc_state = SCTP_ADDR_CONFIRMED; > else > spc_state = SCTP_ADDR_AVAILABLE; Right, yet for one bisecting the kernel, a checkout on this patch will see this change regardless of patch 2. Patches 1 and 2 could be swapped to avoid this situation. > > > > > > transport->state = SCTP_ACTIVE; > > > break; > > > > > > @@ -817,19 +809,18 @@ void sctp_assoc_control_transport(struct sctp_association *asoc, > > > * to inactive state. Also, release the cached route since > > > * there may be a better route next time. > > > */ > > > - if (transport->state != SCTP_UNCONFIRMED) > > > + if (transport->state != SCTP_UNCONFIRMED) { > > > transport->state = SCTP_INACTIVE; > > > - else { > > > + spc_state = SCTP_ADDR_UNREACHABLE; > > > + } else { > > > sctp_transport_dst_release(transport); > > > ulp_notify = false; > > > } > > > - > > > - spc_state = SCTP_ADDR_UNREACHABLE; > > > break; > > > > > > case SCTP_TRANSPORT_PF: > > > transport->state = SCTP_PF; > > > - ulp_notify = false; > > > > Again the event should be supressed if PF isn't exposed. > it will be suppressed after Patch 2/5: > + if (asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE) > + ulp_notify = false; > + else > + spc_state = SCTP_ADDR_POTENTIALLY_FAILED; > break; Same here. > > > > > > + spc_state = SCTP_ADDR_POTENTIALLY_FAILED; > > > break; > > > > > > default: > > > -- > > > 2.1.0 > > > > I also haven't spotted where the test that the application has actually enabled > > state transition events is in the code. > all events will be created, but dropped in sctp_ulpq_tail_event() when trying > to deliver up: > > /* Check if the user wishes to receive this event. */ > if (!sctp_ulpevent_is_enabled(event, ulpq->asoc->subscribe)) > goto out_free; > > > I'd have thought it would be anything is built and allocated. > > > > David > > > > - > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > > Registration No: 1397386 (Wales) > > >
On Fri, Oct 25, 2019 at 11:22 AM Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > > On Sat, Oct 19, 2019 at 04:55:01PM +0800, Xin Long wrote: > > > > @@ -801,14 +801,6 @@ void sctp_assoc_control_transport(struct sctp_association *asoc, > > > > spc_state = SCTP_ADDR_CONFIRMED; > > > > else > > > > spc_state = SCTP_ADDR_AVAILABLE; > > > > - /* Don't inform ULP about transition from PF to > > > > - * active state and set cwnd to 1 MTU, see SCTP > > > > - * Quick failover draft section 5.1, point 5 > > > > - */ > > > > - if (transport->state == SCTP_PF) { > > > > - ulp_notify = false; > > > > - transport->cwnd = asoc->pathmtu; > > > > - } > > > > > > This is wrong. > > > If the old state is PF and the application hasn't exposed PF the event should be > > > ignored. > > yeps, in Patch 2/5: > > + if (transport->state == SCTP_PF && > > + asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE) > > + ulp_notify = false; > > + else if (transport->state == SCTP_UNCONFIRMED && > > + error == SCTP_HEARTBEAT_SUCCESS) > > spc_state = SCTP_ADDR_CONFIRMED; > > else > > spc_state = SCTP_ADDR_AVAILABLE; > > Right, yet for one bisecting the kernel, a checkout on this patch will > see this change regardless of patch 2. Patches 1 and 2 could be > swapped to avoid this situation. > > > > > > > > > > transport->state = SCTP_ACTIVE; > > > > break; > > > > > > > > @@ -817,19 +809,18 @@ void sctp_assoc_control_transport(struct sctp_association *asoc, > > > > * to inactive state. Also, release the cached route since > > > > * there may be a better route next time. > > > > */ > > > > - if (transport->state != SCTP_UNCONFIRMED) > > > > + if (transport->state != SCTP_UNCONFIRMED) { > > > > transport->state = SCTP_INACTIVE; > > > > - else { > > > > + spc_state = SCTP_ADDR_UNREACHABLE; > > > > + } else { > > > > sctp_transport_dst_release(transport); > > > > ulp_notify = false; > > > > } > > > > - > > > > - spc_state = SCTP_ADDR_UNREACHABLE; > > > > break; > > > > > > > > case SCTP_TRANSPORT_PF: > > > > transport->state = SCTP_PF; > > > > - ulp_notify = false; > > > > > > Again the event should be supressed if PF isn't exposed. > > it will be suppressed after Patch 2/5: > > + if (asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE) > > + ulp_notify = false; > > + else > > + spc_state = SCTP_ADDR_POTENTIALLY_FAILED; > > break; > > Same here. okay, will swap them. thanks. > > > > > > > > > > + spc_state = SCTP_ADDR_POTENTIALLY_FAILED; > > > > break; > > > > > > > > default: > > > > -- > > > > 2.1.0 > > > > > > I also haven't spotted where the test that the application has actually enabled > > > state transition events is in the code. > > all events will be created, but dropped in sctp_ulpq_tail_event() when trying > > to deliver up: > > > > /* Check if the user wishes to receive this event. */ > > if (!sctp_ulpevent_is_enabled(event, ulpq->asoc->subscribe)) > > goto out_free; > > > > > I'd have thought it would be anything is built and allocated. > > > > > > David > > > > > > - > > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > > > Registration No: 1397386 (Wales) > > > > >
On Fri, Oct 25, 2019 at 11:22 AM Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > > Hi, > > Sorry for the long delay on this review. > > On Mon, Oct 14, 2019 at 02:14:44PM +0800, Xin Long wrote: > > SCTP Quick failover draft section 5.1, point 5 has been removed > > from rfc7829. Instead, "the sender SHOULD (i) notify the Upper > > Layer Protocol (ULP) about this state transition", as said in > > section 3.2, point 8. > > > > So this patch is to add SCTP_ADDR_POTENTIALLY_FAILED, defined > > in section 7.1, "which is reported if the affected address > > becomes PF". Also remove transport cwnd's update when moving > > from PF back to ACTIVE , which is no longer in rfc7829 either. > > > > v1->v2: > > - no change > > v2->v3: > > - define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > --- > > include/uapi/linux/sctp.h | 2 ++ > > net/sctp/associola.c | 17 ++++------------- > > 2 files changed, 6 insertions(+), 13 deletions(-) > > > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > > index 6bce7f9..f4ab7bb 100644 > > --- a/include/uapi/linux/sctp.h > > +++ b/include/uapi/linux/sctp.h > > @@ -410,6 +410,8 @@ enum sctp_spc_state { > > SCTP_ADDR_ADDED, > > SCTP_ADDR_MADE_PRIM, > > SCTP_ADDR_CONFIRMED, > > + SCTP_ADDR_POTENTIALLY_FAILED, > > +#define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED > > }; > > > > > > diff --git a/net/sctp/associola.c b/net/sctp/associola.c > > index 1ba893b..4f9efba 100644 > > --- a/net/sctp/associola.c > > +++ b/net/sctp/associola.c > > @@ -801,14 +801,6 @@ void sctp_assoc_control_transport(struct sctp_association *asoc, > > While at here, dealing with spc_state, please seize the moment and > initialize it to the enum instead: > > @@ -787,7 +787,7 @@ void sctp_assoc_control_transport(struct sctp_association *asoc, > sctp_sn_error_t error) > { > bool ulp_notify = true; > - int spc_state = 0; > + int spc_state = SCTP_ADDR_AVAILABLE; > > > > spc_state = SCTP_ADDR_CONFIRMED; > > else > > spc_state = SCTP_ADDR_AVAILABLE; > > This else could be removed (equals to initial value). yes, will improve it. > > > - /* Don't inform ULP about transition from PF to > > - * active state and set cwnd to 1 MTU, see SCTP > > - * Quick failover draft section 5.1, point 5 > > - */ > > - if (transport->state == SCTP_PF) { > > - ulp_notify = false; > > - transport->cwnd = asoc->pathmtu; > > - } > > transport->state = SCTP_ACTIVE; > > break; > > > > @@ -817,19 +809,18 @@ void sctp_assoc_control_transport(struct sctp_association *asoc, > > * to inactive state. Also, release the cached route since > > * there may be a better route next time. > > */ > > - if (transport->state != SCTP_UNCONFIRMED) > > + if (transport->state != SCTP_UNCONFIRMED) { > > transport->state = SCTP_INACTIVE; > > - else { > > + spc_state = SCTP_ADDR_UNREACHABLE; > > + } else { > > sctp_transport_dst_release(transport); > > ulp_notify = false; > > } > > - > > - spc_state = SCTP_ADDR_UNREACHABLE; > > break; > > > > case SCTP_TRANSPORT_PF: > > transport->state = SCTP_PF; > > - ulp_notify = false; > > + spc_state = SCTP_ADDR_POTENTIALLY_FAILED; > > break; > > > > default: > > -- > > 2.1.0 > >
diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h index 6bce7f9..f4ab7bb 100644 --- a/include/uapi/linux/sctp.h +++ b/include/uapi/linux/sctp.h @@ -410,6 +410,8 @@ enum sctp_spc_state { SCTP_ADDR_ADDED, SCTP_ADDR_MADE_PRIM, SCTP_ADDR_CONFIRMED, + SCTP_ADDR_POTENTIALLY_FAILED, +#define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED }; diff --git a/net/sctp/associola.c b/net/sctp/associola.c index 1ba893b..4f9efba 100644 --- a/net/sctp/associola.c +++ b/net/sctp/associola.c @@ -801,14 +801,6 @@ void sctp_assoc_control_transport(struct sctp_association *asoc, spc_state = SCTP_ADDR_CONFIRMED; else spc_state = SCTP_ADDR_AVAILABLE; - /* Don't inform ULP about transition from PF to - * active state and set cwnd to 1 MTU, see SCTP - * Quick failover draft section 5.1, point 5 - */ - if (transport->state == SCTP_PF) { - ulp_notify = false; - transport->cwnd = asoc->pathmtu; - } transport->state = SCTP_ACTIVE; break; @@ -817,19 +809,18 @@ void sctp_assoc_control_transport(struct sctp_association *asoc, * to inactive state. Also, release the cached route since * there may be a better route next time. */ - if (transport->state != SCTP_UNCONFIRMED) + if (transport->state != SCTP_UNCONFIRMED) { transport->state = SCTP_INACTIVE; - else { + spc_state = SCTP_ADDR_UNREACHABLE; + } else { sctp_transport_dst_release(transport); ulp_notify = false; } - - spc_state = SCTP_ADDR_UNREACHABLE; break; case SCTP_TRANSPORT_PF: transport->state = SCTP_PF; - ulp_notify = false; + spc_state = SCTP_ADDR_POTENTIALLY_FAILED; break; default:
SCTP Quick failover draft section 5.1, point 5 has been removed from rfc7829. Instead, "the sender SHOULD (i) notify the Upper Layer Protocol (ULP) about this state transition", as said in section 3.2, point 8. So this patch is to add SCTP_ADDR_POTENTIALLY_FAILED, defined in section 7.1, "which is reported if the affected address becomes PF". Also remove transport cwnd's update when moving from PF back to ACTIVE , which is no longer in rfc7829 either. v1->v2: - no change v2->v3: - define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED Signed-off-by: Xin Long <lucien.xin@gmail.com> --- include/uapi/linux/sctp.h | 2 ++ net/sctp/associola.c | 17 ++++------------- 2 files changed, 6 insertions(+), 13 deletions(-)