Based on comments from Vlad I have prepared a new patch to replace
sinfo_timetolive with the newer sinfo_pr_policy and sinfo_pr_value
fields.
On Wed, Dec 24, 2008 at 1:31 AM, Vlad Yasevich
<vladislav.yasevich@xxxxxx> wrote:
> Horacio Sanson wrote:
>>>> These values, except for SCTP_EOF, are exactly the same values used in
>>>> the FreeBSD implementation. This scheme allows the lower byte to be
>>>> used for sctp_sinfo_pr_policy values.
>>>>
>>>> I modified all these flags so they coincide with FreeBSD
>>>> implementation that is also the same code base used for MacOSX and
>>>> Windows implementations to avoid any incompatibilities between
>>>> implementations. At least sinfo_pr_policy is transported between peers
>>>> in an association and having standard values is a good idea.
>>> Buy you don't use it as such. You have a separate __u16 defined.to carry
>>> pr_policy. So before we had a structure:
>>>
>>> struct sctp_sndrcvinfo {
>>> __u16 sinfo_stream;
>>> __u16 sinfo_ssn;
>>> __u16 sinfo_flags;
>>> <---- 16bit memory hole
>>> __u32 sinfo_ppid;
>>> __u32 sinfo_context;
>>> __u32 sinfo_timetolive;
>>> __u32 sinfo_tsn;
>>> __u32 sinfo_cumtsn;
>>> sctp_assoc_t sinfo_assoc_id;
>>> };
>>>
>>> and now we have a structure
>>> struct sctp_sndrcvinfo {
>>> __u16 sinfo_stream;
>>> __u16 sinfo_ssn;
>>> __u16 sinfo_flags;
>>> __u16 sinfo_pr_policy; <---- hole filled
>>> __u32 sinfo_ppid;
>>> __u32 sinfo_context;
>>> __u32 sinfo_pr_value;
>>> __u32 sinfo_tsn;
>>> __u32 sinfo_cumtsn;
>>> sctp_assoc_t sinfo_assoc_id;
>>> };
>>>
>>> So, I am trying to find the reason for shifting the flags. Also, this shift
>>> would require all the application to recompile since they would suddenly start
>>> providing wrong flag values to the kernel (broken ABI).
>>>
>>
>> The idea of the shift, and this is how is done in FreeBSD, is to allow
>> both sinfo_flags and sinfo_pr_policy flags to be specified when using
>> the sctp_sendmsg() call. For example this would allow a call like:
>>
>> sctp_sendmsg(sock,
>> msg, msglen,
>> toaddr, tolen,
>> 0,
>> SCTP_UNORDERED|SCTP_PR_SCTP_TTL, // <-- sinfo_flags
>> and sinfo_pr_policy
>> 0, 3000, 0);
>>
>> The latest socket api draft does not mention the sinfo_pr_policy flags
>> as possible values for the flags argument in sctp_sendmsg() but I
>> believe this is a mistake as it makes no sense to allow users to
>> specify a pr_value but give no way to specify the corresponding
>> pr_policy in the same call. So even if the socket api draft states
>> that sinfo_flags and sinfo_pr_policy are separate fields in the
>> sctp_sndrcvinfo struct, there is a reason to have their values shifted
>> so they can be combined in a single integer variable.
>>
>
> Ok, so you don't really need to shift this. You simply need to define
> the partial reliability policy as flags so they can be passed sctp_sendmsg().
>
> This is a problem with retrofitting the existing api that I'll bring up.
>
>>
>> To avoid breaking the ABI what would be a good approach?? I though
>> maybe doing this:
>>
>> enum sctp_sinfo_flags {
>> SCTP_UNORDERED = 1, /* Send/receive message unordered. */
>> SCTP_ADDR_OVER = 2, /* Override the primary destination. */
>> SCTP_ABORT = 4, /* Send an ABORT message to the peer. */
>> SCTP_PR_NONE = 8, /* Reliable transmission */
>> SCTP_PR_TTL = 16, /* Timed partial reliability */
>> SCTP_EOF = MSG_FIN, /* Initiate graceful shutdown process. (0x0200) */
>> };
>>
>> /* this for draft compatibility */
>> #define SCTP_PR_SCTP_NONE SCTP_PR_NONE
>> #define SCTP_PR_SCTP_TTL SCTP_PR_TTL
>>
>>
>> But allowing the SCTP_PR_* flags to coexist in the same integer is not
>> a good idea because these flags are exclusive.... if for some reason
>> the user provides both SCTP_PR_NONE and SCTP_PR_TTL at the same time
>> what should the implementation interpret? this is not addressed in the
>> draft either. The other option would be to reserve the higher 16bits
>> for PR policy flags and the lower 16bits for sinfo_flags:
>>
>
> The spec doesn't address at all the ability to pass pr policy values to sctp_sendmsg().
>
In the end I opted to use this approach of adding the SCTP_PR_SCTP_*
constants to the sinfo_flags enum. Using the upper 16 bits causes
a lot of trouble when splitting and converting it to a 16bit value
needed to fit in the sinfo_pr_policy field.
>
>> enum sctp_sinfo_flags {
>> ^ISCTP_UNORDERED = 1, /* Send/receive message unordered. */
>> ^ISCTP_ADDR_OVER = 2, /* Overrde the primary destination. */
>> ^ISCTP_ABORT = 4, /* Send an ABORT message to the peer. */
>> ^ISCTP_EOF = MSG_FIN, /* Initiate graceful shutdown process. (0x0200) */
>> ^ISCTP_PR_NONE = 0x010000, /* Reliable transmission */
>> ^ISCTP_PR_TTL = 0x020000, /* Timed partial reliability */
>> };
>>
>> but this assumes that integers are always 32bits long and I am not
>> sure this is a good assumption to take.
>
> This is safe because the flags argument to sctp_sendmsg() is defined as uint32_t and
> you are guaranteed to have 32 bits there.
>
> Also, notice that this split will need to be taken care of in libsctp, since that is
> where sctp_sendmsg() is defined. That function will have to split the flags into
> sinfo_flags and sinfo_pr_policy. This means that we'll need a 2 ways define policy:
> as a flag, and as a direct policy.
>
> The kernel doesn't need to know anything about the split and can treat the 'flag' policy
> values as errors.
>
> I think BSD shifted the flags so that it could get way without dual definitions, but they
> broke the ABI by doing so. I don't know how they could get away with that.
>
>>
>
>>>> I need the ability to set sinfo_pr_policy to SCTP_PR_SCTP_TTL and
>>>> sinfo_pr_value equal to zero as a way to enable a completely
>>>> unreliable service (i.e. no retransmissions at all), similar to UDP
>>>> but with congestion control (maybe DCCP emulation??).
>>> OK. So you need to have a default case, where the expires_policy is
>>> junk and in that case you verify pr_value before setting msg->expires_policy.
>>>
>>> In essence, you need to cover the case of user using the new structure as
>>> well as using an old structure that had a memory hole.
>>>
>>
>>
>>
>>>> I believe this functionality added to unordered delivery can be useful
>>>> for several applications and allows SCTP to cover all the reliability
>>>> spectrum from ordered reliable, ordered unreliable, unordered
>>>> reliable, unordered unreliable, ordered partial reliable and unordered
>>>> partial reliable.
>>>>
>>>> Another alternative would be to define a new partial reliability
>>>> profile, something like SCTP_PR_SCTP_UNR, that forces sinfo_pr_value
>>>> to zero and never retransmits.
>>> I think what you are looking for a retransmit policy (SCTP_PR_RTX) with the
>>> number of retransmits set 0. You don't want to do this as a timer policy with
>>> a timeout of 0 milliseconds, because you may end up discarding packets
>>> as fast as your app can generate them and never send anything.
>>>
>>
>> I have this SCTP_PR_RTX functionality already implemented as a
>> separate patch. Once my first
>> patch gets approval I will submit it for review.
>>
>> If I understand right then something like this would be ok??
>>
>> switch(sinfo->sinfo_pr_policy) {
>>
>> case SCTP_PR_NONE:
>> msg->expires_policy = SCTP_PR_NONE;
>> msg->expires_at = 0;
>> break;
>> case SCTP_PR_RTX:
>> msg->expires_policy = SCTP_PR_RTX;
>> msg->expires_at = sinfo->sinfo_pr_value;
>> break;
>> case SCTP_PR_TTL:
>> default:
>> if(pr_value <= 0) {
>
> I don't think pr_value can be less then 0 since it's defined a unsigned.
fixed.
>
>> msg->expires_policy = SCTP_PR_NONE;
>> msg->expires_at = 0;
>> } else {
>> msg->expires_policy = SCTP_PR_TTL;
>> msg->expires_at = jiffies + msecs_to_jiffies(sinfo->sinfo_pr_value);
>> }
>> break;
>> }
>>
>
> Yes.
>
great, I use this switch statement now.
>>
>>>> About validating sinfo_pr_value and sinfo_pr_policy I added validating
>>>> code on the sctp_msghdr_parse method. I think this is the best place
>>>> to make this validation. This would also make sure that the user gets
>>>> an error message if the sinfo_pr_value or sinfo_pr_policy values are
>>>> not initialized correctly (me thinks).
>>> No, the user may be using an old structure and and old api/abi. We can't break
>>> them. This has been my gripe with this draft for a long time... it keeps
>>> breaking ABI and requiring these ugly workarounds.
>>>
>>> You can log a rate limited warning, but you can't error out since that will
>>> break applications.
>>>
>>> BTW, why are the constants SCTP_PR_SCTP_*. Having SCTP there once should be
>>> enough. We can define BSD compatibility stuff, but having it clean in the kernel.
>>>
>>
>> Once the values I can use for the SCTP_PR_* flags is established I
>> will think on the validation. And about the constants having SCTP
>> twice in them, well I am just following the draft as it is written.
>
> Ah... I should have looked.
>
> I think this is getting cleared up. I'll get a comment to the rest
> of the authors and the wG about specifying policy in sctp_sendmsg() and let
> you know the outcome.
>
hope this time I got it right....
Horacio
> -vlad
>
>>
>> regards
>> Horacio
>>
>>> -vlad
>>>
>>
>
>
From 75b5677399dc58d74dc7cfa04b4d6f37ee705ebd Mon Sep 17 00:00:00 2001
From: Horacio Sanson <horacio@xxxxxxxxxxxxxxxxxx>
Date: Wed, 31 Dec 2008 00:52:41 +0900
Subject: [PATCH] lksctp: replace sinfo_timetolive with sinfo_pr_value.
Modified all relevant structures in all header and source files to
replace the deprecated sinfo_timetolive field with the newer
sinfo_pr_policy and sinfo_pr_value.
Signed-off-by: Horacio Sanson <horacio@xxxxxxxxxxxxxxxxxx>
---
include/net/sctp/structs.h | 10 ++++++----
include/net/sctp/user.h | 5 ++++-
net/sctp/associola.c | 3 ++-
net/sctp/chunk.c | 39 ++++++++++++++++++++++++++-------------
net/sctp/output.c | 2 +-
net/sctp/socket.c | 29 +++++++++++++++++++----------
net/sctp/ulpevent.c | 3 ++-
7 files changed, 60 insertions(+), 31 deletions(-)
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 9661d7b..f7ec249 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -280,9 +280,10 @@ struct sctp_sock {
/* Various Socket Options. */
__u16 default_stream;
__u32 default_ppid;
+ __u16 default_pr_policy;
__u16 default_flags;
__u32 default_context;
- __u32 default_timetolive;
+ __u32 default_pr_value;
__u32 default_rcv_context;
int max_burst;
@@ -626,13 +627,13 @@ struct sctp_datamsg {
struct list_head track;
/* Reference counting. */
atomic_t refcnt;
+ /* Policy used to determine how expires_at is interpreted */
+ unsigned long expires_policy;
/* When is this message no longer interesting to the peer? */
unsigned long expires_at;
/* Did the messenge fail to send? */
int send_error;
char send_failed;
- /* Control whether chunks from this message can be abandoned. */
- char can_abandon;
};
struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *,
@@ -1759,9 +1760,10 @@ struct sctp_association {
/* Default send parameters. */
__u16 default_stream;
__u16 default_flags;
+ __u16 default_pr_policy;
__u32 default_ppid;
__u32 default_context;
- __u32 default_timetolive;
+ __u32 default_pr_value;
/* Default receive parameters */
__u32 default_rcv_context;
diff --git a/include/net/sctp/user.h b/include/net/sctp/user.h
index f205b10..bcdb01e 100644
--- a/include/net/sctp/user.h
+++ b/include/net/sctp/user.h
@@ -183,9 +183,10 @@ struct sctp_sndrcvinfo {
__u16 sinfo_stream;
__u16 sinfo_ssn;
__u16 sinfo_flags;
+ __u16 sinfo_pr_policy;
__u32 sinfo_ppid;
__u32 sinfo_context;
- __u32 sinfo_timetolive;
+ __u32 sinfo_pr_value;
__u32 sinfo_tsn;
__u32 sinfo_cumtsn;
sctp_assoc_t sinfo_assoc_id;
@@ -202,6 +203,8 @@ enum sctp_sinfo_flags {
SCTP_UNORDERED = 1, /* Send/receive message unordered. */
SCTP_ADDR_OVER = 2, /* Override the primary destination. */
SCTP_ABORT=4, /* Send an ABORT message to the peer. */
+ SCTP_PR_SCTP_NONE=8, /* Reliable transmission */
+ SCTP_PR_SCTP_TTL=16, /* Timed partial reliability */
SCTP_EOF=MSG_FIN, /* Initiate graceful shutdown process. */
};
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index f4b2304..c178139 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -302,7 +302,8 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
asoc->default_ppid = sp->default_ppid;
asoc->default_flags = sp->default_flags;
asoc->default_context = sp->default_context;
- asoc->default_timetolive = sp->default_timetolive;
+ asoc->default_pr_policy = sp->default_pr_policy;
+ asoc->default_pr_value = sp->default_pr_value;
asoc->default_rcv_context = sp->default_rcv_context;
/* AUTH related initializations */
diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
index 1748ef9..33141be 100644
--- a/net/sctp/chunk.c
+++ b/net/sctp/chunk.c
@@ -56,7 +56,7 @@ static void sctp_datamsg_init(struct sctp_datamsg *msg)
atomic_set(&msg->refcnt, 1);
msg->send_failed = 0;
msg->send_error = 0;
- msg->can_abandon = 0;
+ msg->expires_policy = SCTP_PR_SCTP_NONE;
msg->expires_at = 0;
INIT_LIST_HEAD(&msg->chunks);
}
@@ -170,13 +170,23 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
/* Note: Calculate this outside of the loop, so that all fragments
* have the same expiration.
*/
- if (sinfo->sinfo_timetolive) {
- /* sinfo_timetolive is in milliseconds */
- msg->expires_at = jiffies +
- msecs_to_jiffies(sinfo->sinfo_timetolive);
- msg->can_abandon = 1;
- SCTP_DEBUG_PRINTK("%s: msg:%p expires_at: %ld jiffies:%ld\n",
- __func__, msg, msg->expires_at, jiffies);
+ switch(sinfo->sinfo_pr_policy) {
+ case SCTP_PR_SCTP_NONE:
+ msg->expires_policy = SCTP_PR_SCTP_NONE;
+ msg->expires_at = 0;
+ break;
+ case SCTP_PR_SCTP_TTL:
+ default:
+ if(!sinfo->sinfo_pr_value) {
+ msg->expires_policy = SCTP_PR_SCTP_NONE;
+ msg->expires_at = 0;
+ } else {
+ msg->expires_policy = SCTP_PR_SCTP_TTL;
+ msg->expires_at = jiffies + msecs_to_jiffies(sinfo->sinfo_pr_value);
+ SCTP_DEBUG_PRINTK("%s: msg:%p expires_at: %ld jiffies:%ld\n",
+ __func__, msg, msg->expires_at, jiffies);
+ }
+ break;
}
max = asoc->frag_point;
@@ -291,11 +301,14 @@ int sctp_chunk_abandoned(struct sctp_chunk *chunk)
{
struct sctp_datamsg *msg = chunk->msg;
- if (!msg->can_abandon)
- return 0;
-
- if (time_after(jiffies, msg->expires_at))
- return 1;
+ switch(msg->expires_policy) {
+ case SCTP_PR_SCTP_NONE:
+ return 0;
+ case SCTP_PR_SCTP_TTL:
+ if(time_after(jiffies, msg->expires_at))
+ return 1;
+ break;
+ }
return 0;
}
diff --git a/net/sctp/output.c b/net/sctp/output.c
index c3f417f..b344a9d 100644
--- a/net/sctp/output.c
+++ b/net/sctp/output.c
@@ -745,7 +745,7 @@ static sctp_xmit_t sctp_packet_append_data(struct sctp_packet *packet,
asoc->peer.rwnd = rwnd;
/* Has been accepted for transmission. */
if (!asoc->peer.prsctp_capable)
- chunk->msg->can_abandon = 0;
+ chunk->msg->expires_policy = SCTP_PR_SCTP_NONE;
finish:
return retval;
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index a1b9045..c21cb4c 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -1698,7 +1698,8 @@ SCTP_STATIC int sctp_sendmsg(struct kiocb *iocb, struct sock *sk,
default_sinfo.sinfo_flags = asoc->default_flags;
default_sinfo.sinfo_ppid = asoc->default_ppid;
default_sinfo.sinfo_context = asoc->default_context;
- default_sinfo.sinfo_timetolive = asoc->default_timetolive;
+ default_sinfo.sinfo_pr_policy = asoc->default_pr_policy;
+ default_sinfo.sinfo_pr_value = asoc->default_pr_value;
default_sinfo.sinfo_assoc_id = sctp_assoc2id(asoc);
sinfo = &default_sinfo;
}
@@ -2539,7 +2540,7 @@ static int sctp_setsockopt_initmsg(struct sock *sk, char __user *optval, int opt
* in to this call the sctp_sndrcvinfo structure defined in Section
* 5.2.2) The input parameters accepted by this call include
* sinfo_stream, sinfo_flags, sinfo_ppid, sinfo_context,
- * sinfo_timetolive. The user must provide the sinfo_assoc_id field in
+ * sinfo_pr_value. The user must provide the sinfo_assoc_id field in
* to this call if the caller is using the UDP model.
*/
static int sctp_setsockopt_default_send_param(struct sock *sk,
@@ -2563,13 +2564,15 @@ static int sctp_setsockopt_default_send_param(struct sock *sk,
asoc->default_flags = info.sinfo_flags;
asoc->default_ppid = info.sinfo_ppid;
asoc->default_context = info.sinfo_context;
- asoc->default_timetolive = info.sinfo_timetolive;
+ asoc->default_pr_policy = info.sinfo_pr_policy;
+ asoc->default_pr_value = info.sinfo_pr_value;
} else {
sp->default_stream = info.sinfo_stream;
sp->default_flags = info.sinfo_flags;
sp->default_ppid = info.sinfo_ppid;
sp->default_context = info.sinfo_context;
- sp->default_timetolive = info.sinfo_timetolive;
+ sp->default_pr_policy = info.sinfo_pr_policy;
+ sp->default_pr_value = info.sinfo_pr_value;
}
return 0;
@@ -3524,7 +3527,8 @@ SCTP_STATIC int sctp_init_sock(struct sock *sk)
sp->default_ppid = 0;
sp->default_flags = 0;
sp->default_context = 0;
- sp->default_timetolive = 0;
+ sp->default_pr_policy = SCTP_PR_SCTP_NONE;
+ sp->default_pr_value = 0;
sp->default_rcv_context = 0;
sp->max_burst = sctp_max_burst;
@@ -4824,7 +4828,7 @@ static int sctp_getsockopt_adaptation_layer(struct sock *sk, int len,
* in to this call the sctp_sndrcvinfo structure defined in Section
* 5.2.2) The input parameters accepted by this call include
* sinfo_stream, sinfo_flags, sinfo_ppid, sinfo_context,
- * sinfo_timetolive. The user must provide the sinfo_assoc_id field in
+ * sinfo_pr_value. The user must provide the sinfo_assoc_id field in
* to this call if the caller is using the UDP model.
*
* For getsockopt, it get the default sctp_sndrcvinfo structure.
@@ -4854,13 +4858,15 @@ static int sctp_getsockopt_default_send_param(struct sock *sk,
info.sinfo_flags = asoc->default_flags;
info.sinfo_ppid = asoc->default_ppid;
info.sinfo_context = asoc->default_context;
- info.sinfo_timetolive = asoc->default_timetolive;
+ info.sinfo_pr_policy = asoc->default_pr_policy;
+ info.sinfo_pr_value = asoc->default_pr_value;
} else {
info.sinfo_stream = sp->default_stream;
info.sinfo_flags = sp->default_flags;
info.sinfo_ppid = sp->default_ppid;
info.sinfo_context = sp->default_context;
- info.sinfo_timetolive = sp->default_timetolive;
+ info.sinfo_pr_policy = sp->default_pr_policy;
+ info.sinfo_pr_value = sp->default_pr_value;
}
if (put_user(len, optlen))
@@ -6107,10 +6113,13 @@ SCTP_STATIC int sctp_msghdr_parse(const struct msghdr *msg,
(struct sctp_sndrcvinfo *)CMSG_DATA(cmsg);
/* Minimally, validate the sinfo_flags. */
- if (cmsgs->info->sinfo_flags &
+/*
+if (cmsgs->info->sinfo_flags &
~(SCTP_UNORDERED | SCTP_ADDR_OVER |
- SCTP_ABORT | SCTP_EOF))
+ SCTP_ABORT | SCTP_EOF | SCTP_PR_SCTP_NONE |
+ SCTP_PR_SCTP_TTL))
return -EINVAL;
+*/
break;
default:
diff --git a/net/sctp/ulpevent.c b/net/sctp/ulpevent.c
index 5f186ca..9517e47 100644
--- a/net/sctp/ulpevent.c
+++ b/net/sctp/ulpevent.c
@@ -948,7 +948,8 @@ void sctp_ulpevent_read_sndrcvinfo(const struct sctp_ulpevent *event,
sinfo.sinfo_context = event->asoc->default_rcv_context;
/* These fields are not used while receiving. */
- sinfo.sinfo_timetolive = 0;
+ sinfo.sinfo_pr_policy = SCTP_PR_SCTP_NONE;
+ sinfo.sinfo_pr_value = 0;
put_cmsg(msghdr, IPPROTO_SCTP, SCTP_SNDRCV,
sizeof(struct sctp_sndrcvinfo), (void *)&sinfo);
--
1.5.6.3
