sctp: Implement Stream Interleave: The I-DATA Chunk Supporting User Message Interleaving
Subsystems (3)
Xin Long
maintainerv2 cover sctp: Implement Stream Interleave: The I-DATA Chunk Supporting User Message Interleaving
Stream Interleave would be Implemented in two Parts:
1. The I-DATA Chunk Supporting User Message Interleaving
2. Interaction with Other SCTP Extensions
Overview in section 1.1 of RFC8260 for Part 1:
This document describes a new chunk carrying payload data called I-DATA. This chunk incorporates the properties of the current SCTP DATA chunk, all the flags and fields except the Stream Sequence Number (SSN), and also adds two new fields in its chunk header -- the Fragment Sequence Number (FSN) and the Message Identifier (MID). The FSN is only used for reassembling all fragments that have the same MID and the same ordering property. The TSN is only used for the reliable transfer in combination with Selective Acknowledgment (SACK) chunks.
In addition, the MID is also used for ensuring ordered delivery instead of using the stream sequence number (the I-DATA chunk omits an SSN).
As the 1st part of Stream Interleave Implementation, this patchset adds an ops framework named sctp_stream_interleave with a bunch of stuff that does lots of things needed somewhere.
Then it defines sctp_stream_interleave_0 to work for normal DATA chunks and sctp_stream_interleave_1 for I-DATA chunks.
With these functions, hundreds of if-else checks for the different process on I-DATA chunks would be avoided. Besides, very few codes could be shared in these two function sets.
In this patchset, it adds some basic variables, structures and socket options firstly, then implement these functions one by one to add the procedures for ordered idata gradually, at last adjusts some codes to make them work for unordered idata.
To make it safe to be implemented and also not break the normal data chunk process, this feature can't be enabled to use until all stream interleave codes are completely accomplished.
v1 -> v2:
- fixed a checkpatch warning that a blank line was missed.
- avoided a kbuild warning reported from gcc-4.9.
Xin Long (12):
sctp: add stream interleave enable members and sockopt
sctp: add asoc intl_enable negotiation during 4 shakehands
sctp: add basic structures and make chunk function for idata
sctp: implement make_datafrag for sctp_stream_interleave
sctp: implement assign_number for sctp_stream_interleave
sctp: implement validate_data for sctp_stream_interleave
sctp: implement ulpevent_data for sctp_stream_interleave
sctp: implement enqueue_event for sctp_stream_interleave
sctp: implement renege_events for sctp_stream_interleave
sctp: implement start_pd for sctp_stream_interleave
sctp: implement abort_pd for sctp_stream_interleave
sctp: add support for the process of unordered idata
include/linux/sctp.h | 20 + include/net/netns/sctp.h | 5 +- include/net/sctp/constants.h | 9 +- include/net/sctp/sctp.h | 4 +- include/net/sctp/sm.h | 15 +- include/net/sctp/stream_interleave.h | 54 ++ include/net/sctp/structs.h | 56 +- include/net/sctp/ulpevent.h | 23 +- include/net/sctp/ulpqueue.h | 10 +- include/uapi/linux/sctp.h | 3 + net/sctp/Makefile | 2 +- net/sctp/associola.c | 2 +- net/sctp/chunk.c | 8 +- net/sctp/output.c | 5 +- net/sctp/sm_make_chunk.c | 45 +- net/sctp/sm_sideeffect.c | 23 +- net/sctp/sm_statefuns.c | 21 +- net/sctp/sm_statetable.c | 3 + net/sctp/socket.c | 130 +++- net/sctp/stream.c | 1 + net/sctp/stream_interleave.c | 1118 ++++++++++++++++++++++++++++++++++ net/sctp/ulpevent.c | 15 +- net/sctp/ulpqueue.c | 23 +- 23 files changed, 1501 insertions(+), 94 deletions(-) create mode 100644 include/net/sctp/stream_interleave.h create mode 100644 net/sctp/stream_interleave.c
Signature · 1 line
2.1.0
Xin Long
maintainerPATCH v2 1/12 sctp: add stream interleave enable members and sockopt
This patch adds intl_enable in asoc and netns, and strm_interleave in sctp_sock to indicate if stream interleave is enabled and supported.
netns intl_enable would be set via procfs, but that is not added yet until all stream interleave codes are completely implemented; asoc intl_enable will be set when doing 4-shakehands.
sp strm_interleave can be set by sockopt SCTP_INTERLEAVING_SUPPORTED which is also added in this patch. This socket option is defined in section 4.3.1 of RFC8260.
Note that strm_interleave can only be set by sockopt when both netns intl_enable and sp frag_interleave are set.
--- include/net/netns/sctp.h | 5 ++- include/net/sctp/structs.h | 2 ++ include/uapi/linux/sctp.h | 1 + net/sctp/socket.c | 88 +++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 94 insertions(+), 2 deletions(-)
include/net/netns/sctp.h +4 -1
@@ -122,9 +122,12 @@ struct netns_sctp {/* Flag to indicate if PR-CONFIG is enabled. */ int reconf_enable; -/* Flag to idicate if SCTP-AUTH is enabled */ +/* Flag to indicate if SCTP-AUTH is enabled */ int auth_enable; +/* Flag to indicate if stream interleave is enabled */ +int intl_enable; +/* * Policy to control SCTP IPv4 address scoping * 0 - Disable IPv4 address scoping
include/net/sctp/structs.h +2 -0
@@ -217,6 +217,7 @@ struct sctp_sock { disable_fragments:1, v4mapped:1, frag_interleave:1, + strm_interleave:1, recvrcvinfo:1, recvnxtinfo:1, data_ready_signalled:1;
@@ -1940,6 +1941,7 @@ struct sctp_association { __u8 need_ecne:1, /* Need to send an ECNE Chunk? */ temp:1, /* Is it a temporary association? */ force_delay:1, + intl_enable:1, prsctp_enable:1, reconf_enable:1;
include/uapi/linux/sctp.h +1 -0
@@ -125,6 +125,7 @@ typedef __s32 sctp_assoc_t;#define SCTP_SOCKOPT_PEELOFF_FLAGS 122#define SCTP_STREAM_SCHEDULER 123#define SCTP_STREAM_SCHEDULER_VALUE 124 +#define SCTP_INTERLEAVING_SUPPORTED 125/* PR-SCTP policies */ #define SCTP_PR_SCTP_NONE 0x0000
net/sctp/socket.c +87 -1
118 lines not on this page · raw message
Signature · 1 line
2.1.0
Xin Long
maintainerPATCH v2 2/12 sctp: add asoc intl_enable negotiation during 4 shakehands
asoc intl_enable will be set when local sp strm_interleave is set and there's I-DATA chunk in init and init_ack extensions, as said in section 2.2.1 of RFC8260.
asoc intl_enable indicates all data will be sent as I-DATA chunks.
--- include/linux/sctp.h | 3 +++ net/sctp/sm_make_chunk.c | 18 ++++++++++++++++-- 2 files changed, 19 insertions(+), 2 deletions(-)
include/linux/sctp.h +3 -0
@@ -102,6 +102,9 @@ enum sctp_cid {/* AUTH Extension Section 4.1 */ SCTP_CID_AUTH =0x0F , +/* sctp ndata 5.1. I-DATA */ +SCTP_CID_I_DATA =0x40 , +/* PR-SCTP Sec 3.2 */ SCTP_CID_FWD_TSN = 0xC0,
net/sctp/sm_make_chunk.c +16 -2
@@ -228,7 +228,7 @@ struct sctp_chunk *sctp_make_init(const struct sctp_association *asoc,struct sctp_inithdr init;union sctp_params addrs;struct sctp_sock *sp; -__u8 extensions[4 ]; +__u8 extensions[5 ];size_t chunksize;__be16 types[2 ];int num_ext =0 ;
@@ -278,6 +278,11 @@ struct sctp_chunk *sctp_make_init(const struct sctp_association *asoc,if (sp->adaptation_ind) chunksize +=sizeof (aiparam); +if (sp->strm_interleave) { + extensions[num_ext] =SCTP_CID_I_DATA ; + num_ext +=1 ; + } + chunksize += vparam_len;/* Account for AUTH related parameters */
@@ -392,7 +397,7 @@ struct sctp_chunk *sctp_make_init_ack(const struct sctp_association *asoc,struct sctp_inithdr initack;union sctp_params addrs;struct sctp_sock *sp; -__u8 extensions[4 ]; +__u8 extensions[5 ];size_t chunksize;int num_ext =0 ;int cookie_len;
@@ -442,6 +447,11 @@ struct sctp_chunk *sctp_make_init_ack(const struct sctp_association *asoc,if (sp->adaptation_ind) chunksize +=sizeof (aiparam); +if (asoc->intl_enable) { + extensions[num_ext] =SCTP_CID_I_DATA ; + num_ext +=1 ; + } +if (asoc->peer.auth_capable) { auth_random = (struct sctp_paramhdr *)asoc->c.auth_random; chunksize +=ntohs (auth_random->length);
@@ -2032,6 +2042,10 @@ static void sctp_process_ext_param(struct sctp_association *asoc,if (net->sctp.addip_enable) asoc->peer.asconf_capable =1 ;break ; +case SCTP_CID_I_DATA : +if (sctp_sk (asoc->base.sk)->strm_interleave) + asoc->intl_enable =1 ; +break ;default :break ; }
Signature · 1 line
2.1.0
Xin Long
maintainerPATCH v2 3/12 sctp: add basic structures and make chunk function for idata
sctp_idatahdr and sctp_idata_chunk are used to define and parse I-DATA chunk format, and sctp_make_idata is a function to build the chunk.
The I-DATA Chunk Format is defined in section 2.1 of RFC8260.
--- include/linux/sctp.h | 17 +++++++++++++++++ include/net/sctp/sm.h | 2 ++ include/net/sctp/structs.h | 1 + net/sctp/sm_make_chunk.c | 6 ++++++ 4 files changed, 26 insertions(+)
include/linux/sctp.h +17 -0
@@ -243,6 +243,23 @@ struct sctp_data_chunk {struct sctp_datahdr data_hdr; }; +struct sctp_idatahdr { +__be32 tsn; +__be16 stream; +__be16 reserved; +__be32 mid; +union { +__u32 ppid; +__be32 fsn; + }; +__u8 payload[0 ]; +}; + +struct sctp_idata_chunk { +struct sctp_chunkhdr chunk_hdr; +struct sctp_idatahdr data_hdr; +}; +/* DATA Chuck Specific Flags */ enum {SCTP_DATA_MIDDLE_FRAG =0x00 ,
include/net/sctp/sm.h +2 -0
@@ -197,6 +197,8 @@ struct sctp_chunk *sctp_make_cookie_ack(const struct sctp_association *asoc,struct sctp_chunk *sctp_make_cwr (const struct sctp_association *asoc,const __u32 lowest_tsn,const struct sctp_chunk *chunk); +struct sctp_chunk *sctp_make_idata (const struct sctp_association *asoc, +__u8 flags,int paylen,gfp_t gfp); struct sctp_chunk *sctp_make_datafrag_empty(struct sctp_association *asoc, const struct sctp_sndrcvinfo *sinfo, int len, const __u8 flags,
include/net/sctp/structs.h +1 -0
@@ -575,6 +575,7 @@ struct sctp_chunk {struct sctp_addiphdr *addip_hdr;struct sctp_fwdtsn_hdr *fwdtsn_hdr;struct sctp_authhdr *auth_hdr; +struct sctp_idatahdr *idata_hdr; } subh;__u8 *chunk_end;
net/sctp/sm_make_chunk.c +6 -0
@@ -1425,6 +1425,12 @@ static struct sctp_chunk *sctp_make_data(const struct sctp_association *asoc,return _sctp_make_chunk (asoc,SCTP_CID_DATA , flags, paylen, gfp); } +struct sctp_chunk *sctp_make_idata (const struct sctp_association *asoc, +__u8 flags,int paylen,gfp_t gfp) +{ +return _sctp_make_chunk (asoc,SCTP_CID_I_DATA , flags, paylen, gfp); +} +static struct sctp_chunk *sctp_make_control (const struct sctp_association *asoc,__u8 type,__u8 flags,int paylen,gfp_t gfp)
Signature · 1 line
2.1.0
Xin Long
maintainerPATCH v2 4/12 sctp: implement make_datafrag for sctp_stream_interleave
To avoid hundreds of checks for the different process on I-DATA chunk, struct sctp_stream_interleave is defined as a group of functions used to replace the codes in some place where it needs to do different job according to if the asoc intl_enabled is set.
With these ops, it only needs to initialize asoc->stream.si with sctp_stream_interleave_0 for normal data if asoc intl_enable is 0, or sctp_stream_interleave_1 for idata if asoc intl_enable is set in sctp_stream_init.
After that, the members in asoc->stream.si can be used directly in some special places without checking asoc intl_enable.
make_datafrag is the first member for sctp_stream_interleave, it's used to make data or idata frags, called in sctp_datamsg_from_user. The old function sctp_make_datafrag_empty needs to be adjust some to fit in this ops.
Note that as idata and data chunks have different length, it also defines data_chunk_len for sctp_stream_interleave to describe the chunk size.
--- include/net/sctp/sm.h | 5 +-- include/net/sctp/stream_interleave.h | 44 ++++++++++++++++++++ include/net/sctp/structs.h | 12 ++++++ net/sctp/Makefile | 2 +- net/sctp/chunk.c | 6 +-- net/sctp/sm_make_chunk.c | 21 ++++------ net/sctp/stream.c | 1 + net/sctp/stream_interleave.c | 79 ++++++++++++++++++++++++++++++++++++ 8 files changed, 149 insertions(+), 21 deletions(-) create mode 100644 include/net/sctp/stream_interleave.h create mode 100644 net/sctp/stream_interleave.c
include/net/sctp/sm.h +2 -3
@@ -199,10 +199,9 @@ struct sctp_chunk *sctp_make_cwr(const struct sctp_association *asoc, const struct sctp_chunk *chunk);struct sctp_chunk *sctp_make_idata (const struct sctp_association *asoc,__u8 flags,int paylen,gfp_t gfp); -struct sctp_chunk *sctp_make_datafrag_empty (struct sctp_association *asoc, +struct sctp_chunk *sctp_make_datafrag_empty (const struct sctp_association *asoc,const struct sctp_sndrcvinfo *sinfo, -int len,const __u8 flags, -__u16 ssn,gfp_t gfp); +int len,__u8 flags,gfp_t gfp);struct sctp_chunk *sctp_make_ecne (const struct sctp_association *asoc,const __u32 lowest_tsn);struct sctp_chunk *sctp_make_sack (const struct sctp_association *asoc);
include/net/sctp/stream_interleave.h new file +44 -0
44 lines not on this page · raw message
include/net/sctp/structs.h +12 -0
@@ -89,6 +89,7 @@ struct sctp_stream;#include <net/sctp/tsnmap.h> #include <net/sctp/ulpevent.h> #include <net/sctp/ulpqueue.h> +#include <net/sctp/stream_interleave.h> /* Structures useful for managing bind/connect. */
@@ -1389,11 +1390,22 @@ struct sctp_stream {struct sctp_stream_out_ext *rr_next; }; }; +struct sctp_stream_interleave *si; };#define SCTP_STREAM_CLOSED 0x00#define SCTP_STREAM_OPEN 0x01 +static inline __u16 sctp_datachk_len (const struct sctp_stream *stream) +{ +return stream->si->data_chunk_len; +} + +static inline __u16 sctp_datahdr_len (const struct sctp_stream *stream) +{ +return stream->si->data_chunk_len -sizeof (struct sctp_chunkhdr ); +} +/* SCTP_GET_ASSOC_STATS counters */ struct sctp_priv_assoc_stats {/* Maximum observed rto in the association during subsequent
net/sctp/Makefile +1 -1
@@ -14,7 +14,7 @@ sctp-y := sm_statetable.o sm_statefuns.o sm_sideeffect.o \ tsnmap.o bind_addr.o socket.o primitive.o \ output.o input.o debug.o stream.o auth.o \ offload.o stream_sched.o stream_sched_prio.o \ - stream_sched_rr.o + stream_sched_rr.o stream_interleave.o sctp_probe-y := probe.o
net/sctp/chunk.c +3 -3
@@ -191,7 +191,7 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,*/ max_data = asoc->pathmtu -sctp_sk (asoc->base.sk)->pf->af->net_header_len - -sizeof (struct sctphdr ) -sizeof (struct sctp_data_chunk ); +sizeof (struct sctphdr ) -sctp_datachk_len (&asoc->stream); max_data =SCTP_TRUNC4 (max_data);/* If the the peer requested that we authenticate DATA chunks
@@ -264,8 +264,8 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc, frag |=SCTP_DATA_SACK_IMM ; } - chunk =sctp_make_datafrag_empty (asoc, sinfo, len, frag, -0 ,GFP_KERNEL ); + chunk = asoc->stream.si->make_datafrag (asoc, sinfo, len, frag, +GFP_KERNEL );if (!chunk) { err = -ENOMEM ; goto errout;
net/sctp/sm_make_chunk.c +7 -14
@@ -721,38 +721,31 @@ struct sctp_chunk *sctp_make_ecne(const struct sctp_association *asoc,/* Make a DATA chunk for the given association from the provided * parameters. However, do not populate the data payload. */ -struct sctp_chunk *sctp_make_datafrag_empty (struct sctp_association *asoc, +struct sctp_chunk *sctp_make_datafrag_empty (const struct sctp_association *asoc,const struct sctp_sndrcvinfo *sinfo, -int data_len,__u8 flags,__u16 ssn, -gfp_t gfp) +int len,__u8 flags,gfp_t gfp) {struct sctp_chunk *retval;struct sctp_datahdr dp; -int chunk_len;/* We assign the TSN as LATE as possible, not here when * creating the chunk. */ - dp.tsn =0 ; +memset (&dp,0 ,sizeof (dp)); + dp.ppid = sinfo->sinfo_ppid; dp.stream =htons (sinfo->sinfo_stream); - dp.ppid = sinfo->sinfo_ppid;/* Set the flags for an unordered send. */ -if (sinfo->sinfo_flags &SCTP_UNORDERED ) { +if (sinfo->sinfo_flags &SCTP_UNORDERED ) flags |=SCTP_DATA_UNORDERED ; - dp.ssn =0 ; - }else - dp.ssn =htons (ssn); - chunk_len =sizeof (dp) + data_len; - retval =sctp_make_data (asoc, flags, chunk_len, gfp); + retval =sctp_make_data (asoc, flags,sizeof (dp) + len, gfp);if (!retval) - goto nodata; +return NULL ; retval->subh.data_hdr =sctp_addto_chunk (retval,sizeof (dp), &dp);memcpy (&retval->sinfo, sinfo,sizeof (struct sctp_sndrcvinfo )); -nodata:return retval; }
net/sctp/stream.c +1 -0
@@ -167,6 +167,7 @@ int sctp_stream_init(struct sctp_stream *stream, __u16 outcnt, __u16 incnt, sched->init (stream); in: +sctp_stream_interleave_init (stream);if (!incnt) goto out;
net/sctp/stream_interleave.c new file +79 -0
79 lines not on this page · raw message
Signature · 1 line
2.1.0
Xin Long
maintainerPATCH v2 5/12 sctp: implement assign_number for sctp_stream_interleave
assign_number is added as a member of sctp_stream_interleave, used to assign ssn for data or mid (message id) for idata, called in sctp_packet_append_data. sctp_chunk_assign_ssn is left as it is, and sctp_chunk_assign_mid is added for sctp_stream_interleave_1.
This procedure is described in section 2.2.2 of RFC8260.
All sizeof(struct sctp_data_chunk) in tx path is replaced with sctp_datachk_len, to make it right for idata as well. And also adjust sctp_chunk_is_data for SCTP_CID_I_DATA.
After this patch, idata can be built and sent in tx path.
Note that if sp strm_interleave is set, it has to wait_connect in sctp_sendmsg, as asoc intl_enable need to be known after 4 shake- hands, to decide if it should use data or idata later. data and idata can't be mixed to send in one asoc.
--- include/net/sctp/constants.h | 9 +++++---- include/net/sctp/sctp.h | 4 ++-- include/net/sctp/sm.h | 2 +- include/net/sctp/stream_interleave.h | 1 + include/net/sctp/structs.h | 18 +++++++++++++++++- net/sctp/output.c | 5 +++-- net/sctp/socket.c | 17 +++++++++++++++-- net/sctp/stream_interleave.c | 37 ++++++++++++++++++++++++++++++++++++ net/sctp/ulpevent.c | 4 ++-- 9 files changed, 83 insertions(+), 14 deletions(-)
include/net/sctp/constants.h +5 -4
@@ -145,12 +145,13 @@ SCTP_SUBTYPE_CONSTRUCTOR(OTHER, enum sctp_event_other, other) SCTP_SUBTYPE_CONSTRUCTOR(PRIMITIVE, enum sctp_event_primitive, primitive) -#define sctp_chunk_is_data (a) (a->chunk_hdr->type = SCTP_CID_DATA) +#define sctp_chunk_is_data (a) (a->chunk_hdr->type = SCTP_CID_DATA || \ + a->chunk_hdr->type = SCTP_CID_I_DATA)/* Calculate the actual data size in a data chunk */ -#define SCTP_DATA_SNDSIZE (c) ((int)((unsigned long)(c->chunk_end)\ - - (unsigned long)(c->chunk_hdr)\ - - sizeof(struct sctp_data_chunk))) +#define SCTP_DATA_SNDSIZE (c) ((int)((unsigned long)(c->chunk_end) - \ + (unsigned long)(c->chunk_hdr) - \ + sctp_datachk_len(&c->asoc->stream)))/* Internal error codes */ enum sctp_ierror {
include/net/sctp/sctp.h +2 -2
@@ -444,13 +444,13 @@ static inline int sctp_frag_point(const struct sctp_association *asoc, int pmtu)int frag = pmtu; frag -= sp->pf->af->net_header_len; - frag -=sizeof (struct sctphdr ) +sizeof (struct sctp_data_chunk ); + frag -=sizeof (struct sctphdr ) +sctp_datachk_len (&asoc->stream);if (asoc->user_frag) frag =min_t (int, frag, asoc->user_frag); frag =SCTP_TRUNC4 (min_t (int, frag,SCTP_MAX_CHUNK_LEN - -sizeof (struct sctp_data_chunk ))); +sctp_datachk_len (&asoc->stream)));return frag; }
include/net/sctp/sm.h +1 -1
@@ -343,7 +343,7 @@ static inline __u16 sctp_data_size(struct sctp_chunk *chunk)__u16 size; size =ntohs (chunk->chunk_hdr->length); - size -=sizeof (struct sctp_data_chunk ); + size -=sctp_datahdr_len (&chunk->asoc->stream);return size; }
include/net/sctp/stream_interleave.h +1 -0
@@ -37,6 +37,7 @@ struct sctp_stream_interleave {struct sctp_chunk *(*make_datafrag)(const struct sctp_association *asoc,const struct sctp_sndrcvinfo *sinfo,int len,__u8 flags,gfp_t gfp); +void (*assign_number)(struct sctp_chunk *chunk); };void sctp_stream_interleave_init (struct sctp_stream *stream);
include/net/sctp/structs.h +17 -1
@@ -399,6 +399,18 @@ void sctp_stream_update(struct sctp_stream *stream, struct sctp_stream *new);#define sctp_ssn_skip (stream, type, sid, ssn) \ ((stream)->type[sid].ssn = ssn + 1) +/* What is the current MID number for this stream? */ +#define sctp_mid_peek (stream, type, sid) \ + ((stream)->type[sid].mid) + +/* Return the next MID number for this stream. */ +#define sctp_mid_next (stream, type, sid) \ + ((stream)->type[sid].mid++) + +/* Skip over this mid and all below. */ +#define sctp_mid_skip (stream, type, sid, mid) \ + ((stream)->type[sid].mid = mid + 1) +/* * Pointers to address related SCTP functions. * (i.e. things that depend on the address family.)
@@ -623,6 +635,7 @@ struct sctp_chunk { __u16 rtt_in_progress:1, /* This chunk used for RTT calc? */ has_tsn:1, /* Does this chunk have a TSN yet? */ has_ssn:1, /* Does this chunk have a SSN yet? */ +#define has_mid has_ssn singleton:1, /* Only chunk in the packet? */ end_of_packet:1, /* Last chunk in the packet? */ ecn_ce_done:1, /* Have we processed the ECN CE bit? */
@@ -1360,7 +1373,10 @@ struct sctp_stream_out_ext { };struct sctp_stream_out { -__u16 ssn; +union { +__u32 mid; +__u16 ssn; + };__u8 state;struct sctp_stream_out_ext *ext; };
net/sctp/output.c +3 -2
@@ -313,6 +313,7 @@ static enum sctp_xmit __sctp_packet_append_chunk(struct sctp_packet *packet,/* We believe that this chunk is OK to add to the packet */ switch (chunk->chunk_hdr->type) {case SCTP_CID_DATA : +case SCTP_CID_I_DATA :/* Account for the data being in the packet */ sctp_packet_append_data (packet, chunk);/* Disallow SACK bundling after DATA. */
@@ -724,7 +725,7 @@ static enum sctp_xmit sctp_packet_can_append_data(struct sctp_packet *packet,* or delay in hopes of bundling a full sized packet. */ if (chunk->skb->len + q->out_qlen > transport->pathmtu - - packet->overhead -sizeof (struct sctp_data_chunk ) -4 ) + packet->overhead -sctp_datachk_len (&chunk->asoc->stream) -4 )/* Enough data queued to fill a packet */ return SCTP_XMIT_OK ;
@@ -759,7 +760,7 @@ static void sctp_packet_append_data(struct sctp_packet *packet, asoc->peer.rwnd = rwnd;sctp_chunk_assign_tsn (chunk); -sctp_chunk_assign_ssn (chunk); + asoc->stream.si->assign_number (chunk); } static enum sctp_xmit sctp_packet_will_fit(struct sctp_packet *packet,
net/sctp/socket.c +15 -2
@@ -2002,7 +2002,20 @@ static int sctp_sendmsg(struct sock *sk, struct msghdr *msg, size_t msg_len)if (err <0 ) goto out_free; - wait_connect = true; +/* If stream interleave is enabled, wait_connect has to be +* done earlier than data enqueue, as it needs to make data +* or idata according to asoc->intl_enable which is set +* after connection is done. +*/ +if (sctp_sk (asoc->base.sk)->strm_interleave) { + timeo =sock_sndtimeo (sk,0 ); + err =sctp_wait_for_connect (asoc, &timeo); +if (err) + goto out_unlock; + }else { + wait_connect = true; + } +pr_debug ("%s: we associated primitively\n" , __func__); }
@@ -3180,7 +3193,7 @@ static int sctp_setsockopt_maxseg(struct sock *sk, char __user *optval, unsignedif (val =0 ) { val = asoc->pathmtu - sp->pf->af->net_header_len; val -=sizeof (struct sctphdr ) + -sizeof (struct sctp_data_chunk ); +sctp_datachk_len (&asoc->stream); } asoc->user_frag = val; asoc->frag_point =sctp_frag_point (asoc, asoc->pathmtu);
net/sctp/stream_interleave.c +37 -0
@@ -57,16 +57,53 @@ static struct sctp_chunk *sctp_make_idatafrag_empty(return retval; } +static void sctp_chunk_assign_mid (struct sctp_chunk *chunk) +{ +struct sctp_stream *stream; +struct sctp_chunk *lchunk; +__u32 cfsn =0 ; +__u16 sid; + +if (chunk->has_mid) +return ; + + sid =sctp_chunk_stream_no (chunk); + stream = &chunk->asoc->stream; + +list_for_each_entry (lchunk, &chunk->msg->chunks, frag_list) { +struct sctp_idatahdr *hdr; + + lchunk->has_mid =1 ; + +if (lchunk->chunk_hdr->flags &SCTP_DATA_UNORDERED ) +continue ; + + hdr = lchunk->subh.idata_hdr; + +if (lchunk->chunk_hdr->flags &SCTP_DATA_FIRST_FRAG ) + hdr->ppid = lchunk->sinfo.sinfo_ppid; +else + hdr->fsn =htonl (cfsn++); + +if (lchunk->chunk_hdr->flags &SCTP_DATA_LAST_FRAG ) + hdr->mid =htonl (sctp_mid_next (stream, out, sid)); +else + hdr->mid =htonl (sctp_mid_peek (stream, out, sid)); + } +} +static struct sctp_stream_interleave sctp_stream_interleave_0 = { .data_chunk_len =sizeof (struct sctp_data_chunk ),/* DATA process functions */ .make_datafrag = sctp_make_datafrag_empty, + .assign_number = sctp_chunk_assign_ssn, };static struct sctp_stream_interleave sctp_stream_interleave_1 = { .data_chunk_len =sizeof (struct sctp_idata_chunk ),/* I-DATA process functions */ .make_datafrag = sctp_make_idatafrag_empty, + .assign_number = sctp_chunk_assign_mid, };void sctp_stream_interleave_init (struct sctp_stream *stream)
net/sctp/ulpevent.c +2 -2
@@ -443,8 +443,8 @@ struct sctp_ulpevent *sctp_ulpevent_make_send_failed( goto fail;/* Pull off the common chunk header and DATA header. */ -skb_pull (skb,sizeof (struct sctp_data_chunk )); - len -=sizeof (struct sctp_data_chunk ); +skb_pull (skb,sctp_datachk_len (&asoc->stream)); + len -=sctp_datachk_len (&asoc->stream);/* Embed the event fields inside the cloned skb. */ event =sctp_skb2event (skb);
Signature · 1 line
2.1.0
Xin Long
maintainerPATCH v2 6/12 sctp: implement validate_data for sctp_stream_interleave
validate_data is added as a member of sctp_stream_interleave, used to validate ssn/chunk type for data or mid (message id)/chunk type for idata, called in sctp_eat_data.
If this check fails, an abort packet will be sent, as said in section 2.2.3 of RFC8260.
It also adds the process for idata in rx path. As Marcelo pointed out, there's no need to add event table for idata, but just share chunk_event_table with data's. It would drop data chunk for idata and drop idata chunk for data by calling validate_data in sctp_eat_data.
As last patch did, it also replaces sizeof(struct sctp_data_chunk) with sctp_datachk_len for rx path.
After this patch, the idata can be accepted and delivered to ulp layer.
--- include/net/sctp/sm.h | 6 ++++++ include/net/sctp/stream_interleave.h | 1 + include/net/sctp/structs.h | 6 +++++- net/sctp/sm_statefuns.c | 21 ++++++++----------- net/sctp/sm_statetable.c | 3 +++ net/sctp/stream_interleave.c | 39 ++++++++++++++++++++++++++++++++++++ 6 files changed, 62 insertions(+), 14 deletions(-)
include/net/sctp/sm.h +6 -0
@@ -359,6 +359,12 @@ static inline __u16 sctp_data_size(struct sctp_chunk *chunk) typecheck(__u32, b) && \ ((__s32)((a) - (b)) <= 0)) +/* Compare two MIDs */ +#define MID_lt (a, b) \ + (typecheck(__u32, a) && \ + typecheck(__u32, b) && \ + ((__s32)((a) - (b)) < 0)) +/* Compare two SSNs */ #define SSN_lt (a,b) \ (typecheck(__u16, a) && \
include/net/sctp/stream_interleave.h +1 -0
@@ -38,6 +38,7 @@ struct sctp_stream_interleave {const struct sctp_sndrcvinfo *sinfo, int len, __u8 flags, gfp_t gfp);void (*assign_number)(struct sctp_chunk *chunk); +bool (*validate_data)(struct sctp_chunk *chunk); };void sctp_stream_interleave_init (struct sctp_stream *stream);
include/net/sctp/structs.h +5 -1
@@ -1382,7 +1382,11 @@ struct sctp_stream_out { };struct sctp_stream_in { -__u16 ssn; +union { +__u32 mid; +__u16 ssn; + }; +__u32 fsn; };struct sctp_stream {
net/sctp/sm_statefuns.c +8 -13
@@ -3013,7 +3013,7 @@ enum sctp_disposition sctp_sf_eat_data_6_2(struct net *net,return sctp_sf_pdiscard (net, ep, asoc, type, arg, commands); } -if (!sctp_chunk_length_valid (chunk,sizeof (struct sctp_data_chunk ))) +if (!sctp_chunk_length_valid (chunk,sctp_datachk_len (&asoc->stream)))return sctp_sf_violation_chunklen (net, ep, asoc, type, arg, commands);
@@ -3034,7 +3034,7 @@ enum sctp_disposition sctp_sf_eat_data_6_2(struct net *net,case SCTP_IERROR_PROTO_VIOLATION :return sctp_sf_abort_violation (net, ep, asoc, chunk, commands, (u8 *)chunk->subh.data_hdr, -sizeof (struct sctp_datahdr )); +sctp_datahdr_len (&asoc->stream));default :BUG (); }
@@ -3133,7 +3133,7 @@ enum sctp_disposition sctp_sf_eat_data_fast_4_4(return sctp_sf_pdiscard (net, ep, asoc, type, arg, commands); } -if (!sctp_chunk_length_valid (chunk,sizeof (struct sctp_data_chunk ))) +if (!sctp_chunk_length_valid (chunk,sctp_datachk_len (&asoc->stream)))return sctp_sf_violation_chunklen (net, ep, asoc, type, arg, commands);
@@ -3150,7 +3150,7 @@ enum sctp_disposition sctp_sf_eat_data_fast_4_4(case SCTP_IERROR_PROTO_VIOLATION :return sctp_sf_abort_violation (net, ep, asoc, chunk, commands, (u8 *)chunk->subh.data_hdr, -sizeof (struct sctp_datahdr )); +sctp_datahdr_len (&asoc->stream));default :BUG (); }
@@ -6244,14 +6244,12 @@ static int sctp_eat_data(const struct sctp_association *asoc,struct sctp_chunk *err;enum sctp_verb deliver;size_t datalen; -u8 ordered =0 ; -u16 ssn, sid;__u32 tsn;int tmp; data_hdr = (struct sctp_datahdr *)chunk->skb->data; chunk->subh.data_hdr = data_hdr; -skb_pull (chunk->skb,sizeof (*data_hdr)); +skb_pull (chunk->skb,sctp_datahdr_len (&asoc->stream)); tsn =ntohl (data_hdr->tsn);pr_debug ("%s: TSN 0x%x\n" , __func__, tsn);
@@ -6299,7 +6297,7 @@ static int sctp_eat_data(const struct sctp_association *asoc,* Actually, allow a little bit of overflow (up to a MTU). */ datalen =ntohs (chunk->chunk_hdr->length); - datalen -=sizeof (struct sctp_data_chunk ); + datalen -=sctp_datachk_len (&asoc->stream); deliver =SCTP_CMD_CHUNK_ULP ;
@@ -6394,7 +6392,6 @@ static int sctp_eat_data(const struct sctp_association *asoc,SCTP_INC_STATS (net,SCTP_MIB_INORDERCHUNKS );if (chunk->asoc) chunk->asoc->stats.iodchunks++; - ordered =1 ; }/* RFC 2960 6.5 Stream Identifier and Stream Sequence Number
@@ -6405,8 +6402,7 @@ static int sctp_eat_data(const struct sctp_association *asoc,* with cause set to "Invalid Stream Identifier" (See Section 3.3.10) * and discard the DATA chunk. */ - sid =ntohs (data_hdr->stream); -if (sid >= asoc->stream.incnt) { +if (ntohs (data_hdr->stream) >= asoc->stream.incnt) {/* Mark tsn as received even though we drop it */ sctp_add_cmd_sf (commands,SCTP_CMD_REPORT_TSN ,SCTP_U32 (tsn));
@@ -6427,8 +6423,7 @@ static int sctp_eat_data(const struct sctp_association *asoc,* SSN is smaller then the next expected one. If it is, it wrapped * and is invalid. */ - ssn =ntohs (data_hdr->ssn); -if (ordered &&SSN_lt (ssn,sctp_ssn_peek (&asoc->stream, in, sid))) +if (!asoc->stream.si->validate_data (chunk))return SCTP_IERROR_PROTO_VIOLATION ;/* Send the data up to the user. Note: Schedule the
net/sctp/sm_statetable.c +3 -0
@@ -985,6 +985,9 @@ static const struct sctp_sm_table_entry *sctp_chunk_event_lookup(if (state >SCTP_STATE_MAX )return &bug; +if (net->sctp.intl_enable && cid =SCTP_CID_I_DATA ) + cid =SCTP_CID_DATA ; +if (cid <=SCTP_CID_BASE_MAX )return &chunk_event_table[cid][state];
net/sctp/stream_interleave.c +39 -0
@@ -92,11 +92,49 @@ static void sctp_chunk_assign_mid(struct sctp_chunk *chunk) } } +static bool sctp_validate_data (struct sctp_chunk *chunk) +{ +const struct sctp_stream *stream; +__u16 sid, ssn; + +if (chunk->chunk_hdr->type !=SCTP_CID_DATA ) +return false; + +if (chunk->chunk_hdr->flags &SCTP_DATA_UNORDERED ) +return true; + + stream = &chunk->asoc->stream; + sid =sctp_chunk_stream_no (chunk); + ssn =ntohs (chunk->subh.data_hdr->ssn); + +return !SSN_lt (ssn,sctp_ssn_peek (stream, in, sid)); +} + +static bool sctp_validate_idata (struct sctp_chunk *chunk) +{ +struct sctp_stream *stream; +__u32 mid; +__u16 sid; + +if (chunk->chunk_hdr->type !=SCTP_CID_I_DATA ) +return false; + +if (chunk->chunk_hdr->flags &SCTP_DATA_UNORDERED ) +return true; + + stream = &chunk->asoc->stream; + sid =sctp_chunk_stream_no (chunk); + mid =ntohl (chunk->subh.idata_hdr->mid); + +return !MID_lt (mid,sctp_mid_peek (stream, in, sid)); +} +static struct sctp_stream_interleave sctp_stream_interleave_0 = { .data_chunk_len =sizeof (struct sctp_data_chunk ),/* DATA process functions */ .make_datafrag = sctp_make_datafrag_empty, .assign_number = sctp_chunk_assign_ssn, + .validate_data = sctp_validate_data, };static struct sctp_stream_interleave sctp_stream_interleave_1 = {
@@ -104,6 +142,7 @@ static struct sctp_stream_interleave sctp_stream_interleave_1 = {/* I-DATA process functions */ .make_datafrag = sctp_make_idatafrag_empty, .assign_number = sctp_chunk_assign_mid, + .validate_data = sctp_validate_idata, };void sctp_stream_interleave_init (struct sctp_stream *stream)
Signature · 1 line
2.1.0
Xin Long
maintainerPATCH v2 7/12 sctp: implement ulpevent_data for sctp_stream_interleave
ulpevent_data is added as a member of sctp_stream_interleave, used to do the most process in ulpq, including to convert data or idata chunk to event, reasm them in reasm queue and put them in lobby queue in right order, and deliver them up to user sk rx queue.
This procedure is described in section 2.2.3 of RFC8260.
It adds most functions for idata here to do the similar process as the old functions for data. But since the details are very different between them, the old functions can not be reused for idata.
event->ssn and event->ppid settings are moved to ulpevent_data from sctp_ulpevent_make_rcvmsg, so that sctp_ulpevent_make_rcvmsg could work for both data and idata.
Note that mid is added in sctp_ulpevent for idata, __packed has to be used for defining sctp_ulpevent, or it would exceeds the skb cb that saves a sctp_ulpevent variable for ulp layer process.
--- include/net/sctp/stream_interleave.h | 2 + include/net/sctp/structs.h | 3 + include/net/sctp/ulpevent.h | 20 +- net/sctp/sm_sideeffect.c | 5 +- net/sctp/stream_interleave.c | 418 +++++++++++++++++++++++++++++++++++ net/sctp/ulpevent.c | 2 - net/sctp/ulpqueue.c | 12 +- 7 files changed, 451 insertions(+), 11 deletions(-)
include/net/sctp/stream_interleave.h +2 -0
@@ -39,6 +39,8 @@ struct sctp_stream_interleave { int len, __u8 flags, gfp_t gfp);void (*assign_number)(struct sctp_chunk *chunk);bool (*validate_data)(struct sctp_chunk *chunk); +int (*ulpevent_data)(struct sctp_ulpq *ulpq, +struct sctp_chunk *chunk,gfp_t gfp); };void sctp_stream_interleave_init (struct sctp_stream *stream);
include/net/sctp/structs.h +3 -0
@@ -411,6 +411,8 @@ void sctp_stream_update(struct sctp_stream *stream, struct sctp_stream *new);#define sctp_mid_skip (stream, type, sid, mid) \ ((stream)->type[sid].mid = mid + 1) +#define sctp_stream_in (asoc, sid) (&(asoc)->stream.in[sid]) +/* * Pointers to address related SCTP functions. * (i.e. things that depend on the address family.)
@@ -1387,6 +1389,7 @@ struct sctp_stream_in {__u16 ssn; };__u32 fsn; +char pd_mode; };struct sctp_stream {
include/net/sctp/ulpevent.h +17 -3
@@ -45,19 +45,29 @@/* A structure to carry information to the ULP (e.g. Sockets API) */ /* Warning: This sits inside an skb.cb[] area. Be very careful of * growing this structure as it is at the maximum limit now. +* +* sctp_ulpevent is saved in sk->cb(48 bytes), whose last 4 bytes +* have been taken by sock_skb_cb, So here it has to use 'packed' +* to make sctp_ulpevent fit into the rest 44 bytes. */ struct sctp_ulpevent {struct sctp_association *asoc;struct sctp_chunk *chunk;unsigned int rmem_len; -__u32 ppid; +union { +__u32 mid; +__u16 ssn; + }; +union { +__u32 ppid; +__u32 fsn; + };__u32 tsn;__u32 cumtsn;__u16 stream; -__u16 ssn;__u16 flags;__u16 msg_flags; -}; +} __packed;/* Retrieve the skb this event sits inside of. */ static inline struct sk_buff *sctp_event2skb (const struct sctp_ulpevent *ev)
@@ -140,6 +150,10 @@ struct sctp_ulpevent *sctp_ulpevent_make_stream_change_event( const struct sctp_association *asoc, __u16 flags, __u32 strchange_instrms, __u32 strchange_outstrms, gfp_t gfp); +struct sctp_ulpevent *sctp_make_reassembled_event ( +struct net *net,struct sk_buff_head *queue, +struct sk_buff *f_frag,struct sk_buff *l_frag); +void sctp_ulpevent_read_sndrcvinfo (const struct sctp_ulpevent *event,struct msghdr *); void sctp_ulpevent_read_rcvinfo(const struct sctp_ulpevent *event,
net/sctp/sm_sideeffect.c +3 -2
@@ -1483,8 +1483,9 @@ static int sctp_cmd_interpreter(enum sctp_event event_type,pr_debug ("%s: sm_sideff: chunk_up:%p, ulpq:%p\n" , __func__, cmd->obj.chunk, &asoc->ulpq); -sctp_ulpq_tail_data (&asoc->ulpq, cmd->obj.chunk, -GFP_ATOMIC ); + asoc->stream.si->ulpevent_data (&asoc->ulpq, + cmd->obj.chunk, +GFP_ATOMIC );break ;case SCTP_CMD_EVENT_ULP :
net/sctp/stream_interleave.c +418 -0
444 lines not on this page · raw message
net/sctp/ulpevent.c +0 -2
@@ -705,8 +705,6 @@ struct sctp_ulpevent *sctp_ulpevent_make_rcvmsg(struct sctp_association *asoc,sctp_ulpevent_receive_data (event, asoc); event->stream =ntohs (chunk->subh.data_hdr->stream); - event->ssn =ntohs (chunk->subh.data_hdr->ssn); - event->ppid = chunk->subh.data_hdr->ppid;if (chunk->chunk_hdr->flags &SCTP_DATA_UNORDERED ) { event->flags |=SCTP_UNORDERED ; event->cumtsn =sctp_tsnmap_get_ctsn (&asoc->peer.tsn_map);
net/sctp/ulpqueue.c +8 -4
@@ -104,6 +104,9 @@ int sctp_ulpq_tail_data(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,if (!event)return -ENOMEM ; + event->ssn =ntohs (chunk->subh.data_hdr->ssn); + event->ppid = chunk->subh.data_hdr->ppid; +/* Do reassembly if needed. */ event =sctp_ulpq_reasm (ulpq, event);
@@ -328,9 +331,10 @@ static void sctp_ulpq_store_reasm(struct sctp_ulpq *ulpq,* payload was fragmented on the way and ip had to reassemble them. * We add the rest of skb's to the first skb's fraglist. */ -static struct sctp_ulpevent *sctp_make_reassembled_event (struct net *net, -struct sk_buff_head *queue,struct sk_buff *f_frag, -struct sk_buff *l_frag) +struct sctp_ulpevent *sctp_make_reassembled_event (struct net *net, +struct sk_buff_head *queue, +struct sk_buff *f_frag, +struct sk_buff *l_frag) {struct sk_buff *pos;struct sk_buff *new =NULL ;
@@ -853,7 +857,7 @@ static struct sctp_ulpevent *sctp_ulpq_order(struct sctp_ulpq *ulpq,struct sctp_stream *stream;/* Check if this message needs ordering. */ -if (SCTP_DATA_UNORDERED & event->msg_flags) +if (event->msg_flags &SCTP_DATA_UNORDERED )return event;/* Note: The stream ID must be verified before this routine. */
Signature · 1 line
2.1.0
Xin Long
maintainerPATCH v2 8/12 sctp: implement enqueue_event for sctp_stream_interleave
enqueue_event is added as a member of sctp_stream_interleave, used to enqueue either data, idata or notification events into user socket rx queue.
It replaces sctp_ulpq_tail_event used in the other places with enqueue_event.
--- include/net/sctp/stream_interleave.h | 2 ++ net/sctp/associola.c | 2 +- net/sctp/chunk.c | 2 +- net/sctp/sm_sideeffect.c | 9 +++++---- net/sctp/socket.c | 2 +- net/sctp/stream_interleave.c | 2 ++ 6 files changed, 12 insertions(+), 7 deletions(-)
include/net/sctp/stream_interleave.h +2 -0
@@ -41,6 +41,8 @@ struct sctp_stream_interleave {bool (*validate_data)(struct sctp_chunk *chunk);int (*ulpevent_data)(struct sctp_ulpq *ulpq,struct sctp_chunk *chunk,gfp_t gfp); +int (*enqueue_event)(struct sctp_ulpq *ulpq, +struct sctp_ulpevent *event); };void sctp_stream_interleave_init (struct sctp_stream *stream);
net/sctp/associola.c +1 -1
@@ -861,7 +861,7 @@ void sctp_assoc_control_transport(struct sctp_association *asoc, event =sctp_ulpevent_make_peer_addr_change (asoc, &addr,0 , spc_state, error,GFP_ATOMIC );if (event) -sctp_ulpq_tail_event (&asoc->ulpq, event); + asoc->stream.si->enqueue_event (&asoc->ulpq, event); }/* Select new active and retran paths. */
net/sctp/chunk.c +1 -1
@@ -124,7 +124,7 @@ static void sctp_datamsg_destroy(struct sctp_datamsg *msg) ev =sctp_ulpevent_make_send_failed (asoc, chunk, sent, error,GFP_ATOMIC );if (ev) -sctp_ulpq_tail_event (&asoc->ulpq, ev); + asoc->stream.si->enqueue_event (&asoc->ulpq, ev); }sctp_chunk_put (chunk);
net/sctp/sm_sideeffect.c +5 -4
@@ -972,7 +972,7 @@ static void sctp_cmd_process_operr(struct sctp_cmd_seq *cmds,if (!ev)return ; -sctp_ulpq_tail_event (&asoc->ulpq, ev); + asoc->stream.si->enqueue_event (&asoc->ulpq, ev);switch (err_hdr->cause) {case SCTP_ERROR_UNKNOWN_CHUNK :
@@ -1058,7 +1058,7 @@ static void sctp_cmd_assoc_change(struct sctp_cmd_seq *commands, asoc->c.sinit_max_instreams, NULL, GFP_ATOMIC);if (ev) -sctp_ulpq_tail_event (&asoc->ulpq, ev); + asoc->stream.si->enqueue_event (&asoc->ulpq, ev); }/* Helper function to generate an adaptation indication event */
@@ -1070,7 +1070,7 @@ static void sctp_cmd_adaptation_ind(struct sctp_cmd_seq *commands, ev =sctp_ulpevent_make_adaptation_indication (asoc,GFP_ATOMIC );if (ev) -sctp_ulpq_tail_event (&asoc->ulpq, ev); + asoc->stream.si->enqueue_event (&asoc->ulpq, ev); }
@@ -1493,7 +1493,8 @@ static int sctp_cmd_interpreter(enum sctp_event event_type,pr_debug ("%s: sm_sideff: event_up:%p, ulpq:%p\n" , __func__, cmd->obj.ulpevent, &asoc->ulpq); -sctp_ulpq_tail_event (&asoc->ulpq, cmd->obj.ulpevent); + asoc->stream.si->enqueue_event (&asoc->ulpq, + cmd->obj.ulpevent);break ;case SCTP_CMD_REPLY :
net/sctp/socket.c +1 -1
@@ -2294,7 +2294,7 @@ static int sctp_setsockopt_events(struct sock *sk, char __user *optval,if (!event)return -ENOMEM ; -sctp_ulpq_tail_event (&asoc->ulpq, event); + asoc->stream.si->enqueue_event (&asoc->ulpq, event); } }
net/sctp/stream_interleave.c +2 -0
@@ -552,6 +552,7 @@ static struct sctp_stream_interleave sctp_stream_interleave_0 = { .assign_number = sctp_chunk_assign_ssn, .validate_data = sctp_validate_data, .ulpevent_data = sctp_ulpq_tail_data, + .enqueue_event = sctp_ulpq_tail_event, };static struct sctp_stream_interleave sctp_stream_interleave_1 = {
@@ -561,6 +562,7 @@ static struct sctp_stream_interleave sctp_stream_interleave_1 = { .assign_number = sctp_chunk_assign_mid, .validate_data = sctp_validate_idata, .ulpevent_data = sctp_ulpevent_idata, + .enqueue_event = sctp_enqueue_event, };void sctp_stream_interleave_init (struct sctp_stream *stream)
Signature · 1 line
2.1.0
Xin Long
maintainerPATCH v2 9/12 sctp: implement renege_events for sctp_stream_interleave
renege_events is added as a member of sctp_stream_interleave, used to renege some old data or idata in reasm or lobby queue properly to free some memory for the new data when there's memory stress.
It defines sctp_renege_events for idata, and leaves sctp_ulpq_renege as it is for data.
--- include/net/sctp/stream_interleave.h | 2 + include/net/sctp/ulpqueue.h | 9 +-- net/sctp/sm_sideeffect.c | 5 +- net/sctp/stream_interleave.c | 109 +++++++++++++++++++++++++++++++++++ net/sctp/ulpqueue.c | 4 +- 5 files changed, 119 insertions(+), 10 deletions(-)
include/net/sctp/stream_interleave.h +2 -0
@@ -43,6 +43,8 @@ struct sctp_stream_interleave { struct sctp_chunk *chunk, gfp_t gfp);int (*enqueue_event)(struct sctp_ulpq *ulpq,struct sctp_ulpevent *event); +void (*renege_events)(struct sctp_ulpq *ulpq, +struct sctp_chunk *chunk,gfp_t gfp); };void sctp_stream_interleave_init (struct sctp_stream *stream);
include/net/sctp/ulpqueue.h +3 -6
@@ -76,11 +76,8 @@ int sctp_clear_pd(struct sock *sk, struct sctp_association *asoc);void sctp_ulpq_skip (struct sctp_ulpq *ulpq,__u16 sid,__u16 ssn);void sctp_ulpq_reasm_flushtsn (struct sctp_ulpq *,__u32 ); -#endif /* __sctp_ulpqueue_h__ */ - - - - - +__u16 sctp_ulpq_renege_list (struct sctp_ulpq *ulpq, +struct sk_buff_head *list,__u16 needed); +#endif /* __sctp_ulpqueue_h__ */
net/sctp/sm_sideeffect.c +3 -2
@@ -1735,8 +1735,9 @@ static int sctp_cmd_interpreter(enum sctp_event event_type,break ;case SCTP_CMD_RENEGE : -sctp_ulpq_renege (&asoc->ulpq, cmd->obj.chunk, -GFP_ATOMIC ); + asoc->stream.si->renege_events (&asoc->ulpq, + cmd->obj.chunk, +GFP_ATOMIC );break ;case SCTP_CMD_SETUP_T4 :
net/sctp/stream_interleave.c +109 -0
127 lines not on this page · raw message
net/sctp/ulpqueue.c +2 -2
@@ -978,8 +978,8 @@ void sctp_ulpq_skip(struct sctp_ulpq *ulpq, __u16 sid, __u16 ssn)sctp_ulpq_reap_ordered (ulpq, sid); } -static __u16 sctp_ulpq_renege_list (struct sctp_ulpq *ulpq, -struct sk_buff_head *list,__u16 needed) +__u16 sctp_ulpq_renege_list (struct sctp_ulpq *ulpq,struct sk_buff_head *list, +__u16 needed) {__u16 freed =0 ;__u32 tsn, last_tsn;
Signature · 1 line
2.1.0
Xin Long
maintainerPATCH v2 10/12 sctp: implement start_pd for sctp_stream_interleave
start_pd is added as a member of sctp_stream_interleave, used to do partial_delivery for data or idata when datalen >= asoc->rwnd in sctp_eat_data. The codes have been done in last patches, but they need to be extracted into start_pd, so that it could be used for SCTP_CMD_PART_DELIVER cmd as well.
--- include/net/sctp/stream_interleave.h | 1 + net/sctp/sm_sideeffect.c | 2 +- net/sctp/stream_interleave.c | 2 ++ 3 files changed, 4 insertions(+), 1 deletion(-)
include/net/sctp/stream_interleave.h +1 -0
@@ -45,6 +45,7 @@ struct sctp_stream_interleave { struct sctp_ulpevent *event);void (*renege_events)(struct sctp_ulpq *ulpq,struct sctp_chunk *chunk,gfp_t gfp); +void (*start_pd)(struct sctp_ulpq *ulpq,gfp_t gfp); };void sctp_stream_interleave_init (struct sctp_stream *stream);
net/sctp/sm_sideeffect.c +1 -1
@@ -1731,7 +1731,7 @@ static int sctp_cmd_interpreter(enum sctp_event event_type,break ;case SCTP_CMD_PART_DELIVER : -sctp_ulpq_partial_delivery (&asoc->ulpq,GFP_ATOMIC ); + asoc->stream.si->start_pd (&asoc->ulpq,GFP_ATOMIC );break ;case SCTP_CMD_RENEGE :
net/sctp/stream_interleave.c +2 -0
@@ -661,6 +661,7 @@ static struct sctp_stream_interleave sctp_stream_interleave_0 = { .ulpevent_data = sctp_ulpq_tail_data, .enqueue_event = sctp_ulpq_tail_event, .renege_events = sctp_ulpq_renege, + .start_pd = sctp_ulpq_partial_delivery, };static struct sctp_stream_interleave sctp_stream_interleave_1 = {
@@ -672,6 +673,7 @@ static struct sctp_stream_interleave sctp_stream_interleave_1 = { .ulpevent_data = sctp_ulpevent_idata, .enqueue_event = sctp_enqueue_event, .renege_events = sctp_renege_events, + .start_pd = sctp_intl_start_pd, };void sctp_stream_interleave_init (struct sctp_stream *stream)
Signature · 1 line
2.1.0
Xin Long
maintainerPATCH v2 11/12 sctp: implement abort_pd for sctp_stream_interleave
abort_pd is added as a member of sctp_stream_interleave, used to abort partial delivery for data or idata, called in sctp_cmd_assoc_failed.
Since stream interleave allows to do partial delivery for each stream at the same time, sctp_intl_abort_pd for idata would be very different from the old function sctp_ulpq_abort_pd for data.
Note that sctp_ulpevent_make_pdapi will support per stream in this patch by adding pdapi_stream and pdapi_seq in sctp_pdapi_event, as described in section 6.1.7 of RFC6458.
--- include/net/sctp/stream_interleave.h | 1 + include/net/sctp/ulpevent.h | 3 +- include/uapi/linux/sctp.h | 2 + net/sctp/sm_sideeffect.c | 2 +- net/sctp/stream_interleave.c | 99 ++++++++++++++++++++++++++++++++++++ net/sctp/ulpevent.c | 9 ++-- net/sctp/ulpqueue.c | 2 +- 7 files changed, 112 insertions(+), 6 deletions(-)
include/net/sctp/stream_interleave.h +1 -0
@@ -46,6 +46,7 @@ struct sctp_stream_interleave {void (*renege_events)(struct sctp_ulpq *ulpq,struct sctp_chunk *chunk,gfp_t gfp);void (*start_pd)(struct sctp_ulpq *ulpq,gfp_t gfp); +void (*abort_pd)(struct sctp_ulpq *ulpq,gfp_t gfp); };void sctp_stream_interleave_init (struct sctp_stream *stream);
include/net/sctp/ulpevent.h +2 -1
@@ -122,7 +122,8 @@ struct sctp_ulpevent *sctp_ulpevent_make_shutdown_event(struct sctp_ulpevent *sctp_ulpevent_make_pdapi (const struct sctp_association *asoc, -__u32 indication,gfp_t gfp); +__u32 indication,__u32 sid,__u32 seq, +__u32 flags,gfp_t gfp);struct sctp_ulpevent *sctp_ulpevent_make_adaptation_indication (const struct sctp_association *asoc,gfp_t gfp);
include/uapi/linux/sctp.h +2 -0
@@ -460,6 +460,8 @@ struct sctp_pdapi_event {__u32 pdapi_length;__u32 pdapi_indication;sctp_assoc_t pdapi_assoc_id; +__u32 pdapi_stream; +__u32 pdapi_seq; };enum {SCTP_PARTIAL_DELIVERY_ABORTED =0 , };
net/sctp/sm_sideeffect.c +1 -1
@@ -632,7 +632,7 @@ static void sctp_cmd_assoc_failed(struct sctp_cmd_seq *commands,struct sctp_chunk *abort;/* Cancel any partial delivery in progress. */ -sctp_ulpq_abort_pd (&asoc->ulpq,GFP_ATOMIC ); + asoc->stream.si->abort_pd (&asoc->ulpq,GFP_ATOMIC );if (event_type =SCTP_EVENT_T_CHUNK && subtype.chunk =SCTP_CID_ABORT ) event = sctp_ulpevent_make_assoc_change(asoc, 0, SCTP_COMM_LOST,
net/sctp/stream_interleave.c +99 -0
117 lines not on this page · raw message
net/sctp/ulpevent.c +6 -3
@@ -730,8 +730,9 @@ struct sctp_ulpevent *sctp_ulpevent_make_rcvmsg(struct sctp_association *asoc,* various events. */ struct sctp_ulpevent *sctp_ulpevent_make_pdapi ( -const struct sctp_association *asoc,__u32 indication, -gfp_t gfp) +const struct sctp_association *asoc, +__u32 indication,__u32 sid,__u32 seq, +__u32 flags,gfp_t gfp) {struct sctp_ulpevent *event;struct sctp_pdapi_event *pd;
@@ -752,7 +753,9 @@ struct sctp_ulpevent *sctp_ulpevent_make_pdapi(* Currently unused. */ pd->pdapi_type =SCTP_PARTIAL_DELIVERY_EVENT ; - pd->pdapi_flags =0 ; + pd->pdapi_flags = flags; + pd->pdapi_stream = sid; + pd->pdapi_seq = seq;/* pdapi_length: 32 bits (unsigned integer) *
net/sctp/ulpqueue.c +1 -1
@@ -1144,7 +1144,7 @@ void sctp_ulpq_abort_pd(struct sctp_ulpq *ulpq, gfp_t gfp) &sctp_sk(sk)->subscribe)) ev =sctp_ulpevent_make_pdapi (ulpq->asoc,SCTP_PARTIAL_DELIVERY_ABORTED , - gfp); +0 ,0 ,0 , gfp);if (ev)__skb_queue_tail (&sk->sk_receive_queue,sctp_event2skb (ev));
Signature · 1 line
2.1.0
Xin Long
maintainerPATCH v2 12/12 sctp: add support for the process of unordered idata
Unordered idata process is more complicated than unordered data:
- It has to add mid into sctp_stream_out to save the next mid value, which is separated from ordered idata's.
- To support pd for unordered idata, another mid and pd_mode need to be added to save the message id and pd state in sctp_stream_in.
- To make unordered idata reasm easier, it adds a new event queue to save frags for idata.
The patch mostly adds the samilar reasm functions for unordered idata as ordered idata's, and also adjusts some other codes on assign_mid, abort_pd and ulpevent_data for idata.
--- include/net/sctp/structs.h | 14 +- include/net/sctp/ulpqueue.h | 1 + net/sctp/socket.c | 23 ++- net/sctp/stream_interleave.c | 377 ++++++++++++++++++++++++++++++++++++++++--- net/sctp/ulpqueue.c | 5 + 5 files changed, 392 insertions(+), 28 deletions(-)
include/net/sctp/structs.h +13 -1
@@ -413,6 +413,14 @@ void sctp_stream_update(struct sctp_stream *stream, struct sctp_stream *new);#define sctp_stream_in (asoc, sid) (&(asoc)->stream.in[sid]) +/* What is the current MID_uo number for this stream? */ +#define sctp_mid_uo_peek (stream, type, sid) \ + ((stream)->type[sid].mid_uo) + +/* Return the next MID_uo number for this stream. */ +#define sctp_mid_uo_next (stream, type, sid) \ + ((stream)->type[sid].mid_uo++) +/* * Pointers to address related SCTP functions. * (i.e. things that depend on the address family.)
@@ -1379,8 +1387,9 @@ struct sctp_stream_out {__u32 mid;__u16 ssn; }; -__u8 state; +__u32 mid_uo;struct sctp_stream_out_ext *ext; +__u8 state; };struct sctp_stream_in {
@@ -1388,8 +1397,11 @@ struct sctp_stream_in {__u32 mid;__u16 ssn; }; +__u32 mid_uo;__u32 fsn; +__u32 fsn_uo;char pd_mode; +char pd_mode_uo; };struct sctp_stream {
include/net/sctp/ulpqueue.h +1 -0
@@ -45,6 +45,7 @@ struct sctp_ulpq {char pd_mode;struct sctp_association *asoc;struct sk_buff_head reasm; +struct sk_buff_head reasm_uo;struct sk_buff_head lobby; };
net/sctp/socket.c +18 -5
@@ -201,6 +201,22 @@ static void sctp_for_each_tx_datachunk(struct sctp_association *asoc,cb (chunk); } +static void sctp_for_each_rx_skb (struct sctp_association *asoc,struct sock *sk, +void (*cb)(struct sk_buff *,struct sock *)) + +{ +struct sk_buff *skb, *tmp; + +sctp_skb_for_each (skb, &asoc->ulpq.lobby, tmp) +cb (skb, sk); + +sctp_skb_for_each (skb, &asoc->ulpq.reasm, tmp) +cb (skb, sk); + +sctp_skb_for_each (skb, &asoc->ulpq.reasm_uo, tmp) +cb (skb, sk); +} +/* Verify that this is a valid address. */ static inline int sctp_verify_addr (struct sock *sk,union sctp_addr *addr,int len)
@@ -1554,6 +1570,7 @@ static void sctp_close(struct sock *sk, long timeout)if (data_was_unread || !skb_queue_empty (&asoc->ulpq.lobby) || !skb_queue_empty (&asoc->ulpq.reasm) || + !skb_queue_empty (&asoc->ulpq.reasm_uo) || (sock_flag (sk,SOCK_LINGER ) && !sk->sk_lingertime)) {struct sctp_chunk *chunk;
@@ -8507,11 +8524,7 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk, } -sctp_skb_for_each (skb, &assoc->ulpq.reasm, tmp) -sctp_skb_set_owner_r_frag (skb, newsk); - -sctp_skb_for_each (skb, &assoc->ulpq.lobby, tmp) -sctp_skb_set_owner_r_frag (skb, newsk); +sctp_for_each_rx_skb (assoc, newsk, sctp_skb_set_owner_r_frag);/* Set the type of socket to indicate that it is peeled off from the * original UDP-style socket or created with the accept() call on a
net/sctp/stream_interleave.c +355 -22
430 lines not on this page · raw message
net/sctp/ulpqueue.c +5 -0
@@ -60,6 +60,7 @@ struct sctp_ulpq *sctp_ulpq_init(struct sctp_ulpq *ulpq, ulpq->asoc = asoc;skb_queue_head_init (&ulpq->reasm); +skb_queue_head_init (&ulpq->reasm_uo);skb_queue_head_init (&ulpq->lobby); ulpq->pd_mode =0 ;
@@ -83,6 +84,10 @@ void sctp_ulpq_flush(struct sctp_ulpq *ulpq)sctp_ulpevent_free (event); } +while ((skb =__skb_dequeue (&ulpq->reasm_uo)) !=NULL ) { + event =sctp_skb2event (skb); +sctp_ulpevent_free (event); + } }/* Dispose of a ulpqueue. */
Signature · 1 line
2.1.0
RE: [PATCHv2 net-next 04/12] sctp: implement make_datafrag for sctp_stream_interleave
In reply to Xin LongFrom: Xin Long
Quoted 1 line
Sent: 08 December 2017 13:04
...
net/sctp/chunk.c struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc, · line 268
Quoted 8 lines
net/sctp/chunk.c +2
-2@@ -264,8 +264,8 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc, frag |=SCTP_DATA_SACK_IMM ; } - chunk =sctp_make_datafrag_empty (asoc, sinfo, len, frag, -0 ,GFP_KERNEL ); + chunk = asoc->stream.si->make_datafrag (asoc, sinfo, len, frag, +GFP_KERNEL );
I know that none of the sctp code is very optimised, but that indirect call is going to be horrid.
David
Marcelo Ricardo Leitner
maintainerRe: [PATCHv2 net-next 04/12] sctp: implement make_datafrag for sctp_stream_interleave
Quoted 7 lines · David Laight
From: Xin Long
Quoted 1 line
Sent: 08 December 2017 13:04
...
Quoted 4 lines
net/sctp/chunk.c +0
-0@@ -264,8 +264,8 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc, frag |=SCTP_DATA_SACK_IMM ; }
Quoted 7 lines
net/sctp/chunk.c struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc, · line 268
Quoted 4 lines
net/sctp/chunk.c +2
-2- chunk =sctp_make_datafrag_empty (asoc, sinfo, len, frag, -0 ,GFP_KERNEL ); + chunk = asoc->stream.si->make_datafrag (asoc, sinfo, len, frag, +GFP_KERNEL );I know that none of the sctp code is very optimised, but that indirect call is going to be horrid.
Yeah.. but there is no way to avoid the double derreference considering we only have the asoc pointer in there and we have to reach the contents of the data chunk operations struct, and the .si part is the same as 'stream' part as it's a constant offset.
Due to the for() in there, we could add a variable to store asoc->stream.si outside the for and then we can do only a single deref inside it. Xin, can you please try and see if the generated code is different?
Other suggestions?
Marcelo
RE: [PATCHv2 net-next 04/12] sctp: implement make_datafrag for sctp_stream_interleave
From: Marcelo Ricardo Leitner
Quoted 18 lines
Sent: 08 December 2017 14:57
Quoted 7 lines · David Laight
From: Xin Long
Quoted 1 line
Sent: 08 December 2017 13:04
...
Quoted 4 lines
net/sctp/chunk.c +0
-0@@ -264,8 +264,8 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc, frag |=SCTP_DATA_SACK_IMM ; }Quoted 7 lines
net/sctp/chunk.c struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc, · line 268
Quoted 4 lines
net/sctp/chunk.c +2
-2- chunk =sctp_make_datafrag_empty (asoc, sinfo, len, frag, -0 ,GFP_KERNEL ); + chunk = asoc->stream.si->make_datafrag (asoc, sinfo, len, frag, +GFP_KERNEL );I know that none of the sctp code is very optimised, but that indirect call is going to be horrid.
Quoted 4 lines
Yeah.. but there is no way to avoid the double derreference considering we only have the asoc pointer in there and we have to reach the contents of the data chunk operations struct, and the .si part is the same as 'stream' part as it's a constant offset.
...
It isn't only the double indirect, the indirect call itself isn't 'fun'.
I think there are other hot paths where you've replaced a sizeof() with a ?: clause.
Caching the result might be much better.
David
'Marcelo Ricardo Leitner'
maintainerRe: [PATCHv2 net-next 04/12] sctp: implement make_datafrag for sctp_stream_interleave
Quoted 19 lines · David Laight
From: Marcelo Ricardo Leitner
Quoted 18 lines
Sent: 08 December 2017 14:57
Quoted 14 lines · David Laight
From: Xin Long
Quoted 1 line
Sent: 08 December 2017 13:04
...
net/sctp/chunk.c struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc, · line 268
Quoted 8 lines
net/sctp/chunk.c +2
-2@@ -264,8 +264,8 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc, frag |=SCTP_DATA_SACK_IMM ; } - chunk =sctp_make_datafrag_empty (asoc, sinfo, len, frag, -0 ,GFP_KERNEL ); + chunk = asoc->stream.si->make_datafrag (asoc, sinfo, len, frag, +GFP_KERNEL );I know that none of the sctp code is very optimised, but that indirect call is going to be horrid.
Quoted 7 lines
Quoted 4 lines
Yeah.. but there is no way to avoid the double derreference considering we only have the asoc pointer in there and we have to reach the contents of the data chunk operations struct, and the .si part is the same as 'stream' part as it's a constant offset.
...
It isn't only the double indirect, the indirect call itself isn't 'fun'.
I meant in this context.
The indirect call is so we don't have to flood the stack with if (old data chunk fmt) {
...
} else {
...
}So instead of this, we now have some key operations identified and wrapped up behind this struct, allowing us to abstract whatever data chunk format it is.
Quoted 4 lines
I think there are other hot paths where you've replaced a
sizeof()with a ?: clause.
Caching the result might be much better.
The only new ?: clause I could find this patchset is on patch 12 and has nothing to do with sizeof().
The sizeof() results are indeed cached, as you can see in patch 4:
+static struct sctp_stream_interleave sctp_stream_interleave_0 = {
+ .data_chunk_len = sizeof(struct sctp_data_chunk),
and the two helpers on it at the begining of the patch.
Marcelo
RE: [PATCHv2 net-next 04/12] sctp: implement make_datafrag for sctp_stream_interleave
From: 'Marcelo Ricardo Leitner'
Quoted 33 lines
Sent: 08 December 2017 15:16
Quoted 19 lines · David Laight
From: Marcelo Ricardo Leitner
Quoted 18 lines
Sent: 08 December 2017 14:57
Quoted 14 lines · David Laight
From: Xin Long
Quoted 1 line
Sent: 08 December 2017 13:04
...
net/sctp/chunk.c struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc, · line 268
Quoted 8 lines
net/sctp/chunk.c +2
-2@@ -264,8 +264,8 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc, frag |=SCTP_DATA_SACK_IMM ; } - chunk =sctp_make_datafrag_empty (asoc, sinfo, len, frag, -0 ,GFP_KERNEL ); + chunk = asoc->stream.si->make_datafrag (asoc, sinfo, len, frag, +GFP_KERNEL );I know that none of the sctp code is very optimised, but that indirect call is going to be horrid.
Quoted 7 lines
Quoted 4 lines
Yeah.. but there is no way to avoid the double derreference considering we only have the asoc pointer in there and we have to reach the contents of the data chunk operations struct, and the .si part is the same as 'stream' part as it's a constant offset.
...
It isn't only the double indirect, the indirect call itself isn't 'fun'.
I meant in this context.
The indirect call is so we don't have to flood the stack with if (old data chunk fmt) {
Quoted 8 lines
... } else { ... }So instead of this, we now have some key operations identified and wrapped up behind this struct, allowing us to abstract whatever data chunk format it is.
Nothing wrong with:
#define foo(asoc, ...) \ if (asoc->new_fmt) \ foo_new(asoc, __VA_LIST__); \ else \ foo_old(asoc, __VA_LIST__);
Quoted 11 lines
Quoted 3 lines
I think there are other hot paths where you've replaced a
sizeof()with a ?: clause.
Caching the result might be much better.The only new ?: clause I could find this patchset is on patch 12 and has nothing to do with
sizeof().The
sizeof()results are indeed cached, as you can see in patch 4:net/sctp/stream_interleave.c new file +2
-0+static struct sctp_stream_interleave sctp_stream_interleave_0 = { + .data_chunk_len =sizeof (struct sctp_data_chunk ),and the two helpers on it at the begining of the patch.
I was getting two bits mixed up.
But the code that reads data_chunk_len is following an awful lot of pointers.
David
'Marcelo Ricardo Leitner'
maintainerRe: [PATCHv2 net-next 04/12] sctp: implement make_datafrag for sctp_stream_interleave
Quoted 39 lines · David Laight
From: 'Marcelo Ricardo Leitner'
Quoted 38 lines
Sent: 08 December 2017 15:16
Quoted 19 lines · David Laight
From: Marcelo Ricardo Leitner
Quoted 18 lines
Sent: 08 December 2017 14:57
Quoted 14 lines · David Laight
From: Xin Long
Quoted 1 line
Sent: 08 December 2017 13:04
...
net/sctp/chunk.c struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc, · line 268
Quoted 8 lines
net/sctp/chunk.c +2
-2@@ -264,8 +264,8 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc, frag |=SCTP_DATA_SACK_IMM ; } - chunk =sctp_make_datafrag_empty (asoc, sinfo, len, frag, -0 ,GFP_KERNEL ); + chunk = asoc->stream.si->make_datafrag (asoc, sinfo, len, frag, +GFP_KERNEL );I know that none of the sctp code is very optimised, but that indirect call is going to be horrid.
Quoted 7 lines
Quoted 4 lines
Yeah.. but there is no way to avoid the double derreference considering we only have the asoc pointer in there and we have to reach the contents of the data chunk operations struct, and the .si part is the same as 'stream' part as it's a constant offset.
...
It isn't only the double indirect, the indirect call itself isn't 'fun'.
I meant in this context.
The indirect call is so we don't have to flood the stack with if (old data chunk fmt) {
... } else { ... }
Quoted 7 lines
Quoted 3 lines
So instead of this, we now have some key operations identified and wrapped up behind this struct, allowing us to abstract whatever data chunk format it is.
Nothing wrong with:
#define foo(asoc, ...) \ if (asoc->new_fmt) \
Not all function pointers in sctp_stream_interleave have asoc as a parameter, so we would have to have something like:
#define foo_asoc(asoc, ...) \ if (asoc->new_fmt) \ ...
#define foo_chunk(chunk, ...) \ if (chunk->asoc->new_fmt) \ ...
#define foo_ulpq(ulpq, ...) \ if (ulpq->asoc->new_fmt) \ ...
and we're pretty much back to double deref.
Maybe some reworking on the parameters could alleviate some of these, but not all.
Quoted 11 lines
foo_new(asoc, __VA_LIST__); \ else \ foo_old(asoc, __VA_LIST__);Quoted 7 lines
Quoted 3 lines
I think there are other hot paths where you've replaced a
sizeof()with a ?: clause.
Caching the result might be much better.The only new ?: clause I could find this patchset is on patch 12 and has nothing to do with
sizeof().
Quoted 7 lines
Quoted 4 lines
The
sizeof()results are indeed cached, as you can see in patch 4:net/sctp/stream_interleave.c new file +2
-0+static struct sctp_stream_interleave sctp_stream_interleave_0 = { + .data_chunk_len =sizeof (struct sctp_data_chunk ),and the two helpers on it at the begining of the patch.
I was getting two bits mixed up.
But the code that readsdata_chunk_lenis following an awful lot of pointers.
From path 4:
max_data = asoc->pathmtu -
sctp_sk(asoc->base.sk)->pf->af->net_header_len -- sizeof(struct sctphdr) - sizeof(struct sctp_data_chunk);
+ sizeof(struct sctphdr) - sctp_datachk_len(&asoc->stream);
You're worried with the double deref in sctp_datachk_len() but on the line right above it we have 4 derefs. There are several other cases of double deref in sctp stack even on hot path. Not sure why you're picking on this one, but ok.
Marcelo
Re: [PATCHv2 net-next 04/12] sctp: implement make_datafrag for sctp_stream_interleave
In reply to Marcelo Ricardo LeitnerQuoted 21 lines · Marcelo Ricardo Leitner
Quoted 7 lines · David Laight
From: Xin Long
Quoted 1 line
Sent: 08 December 2017 13:04
...
Quoted 4 lines
net/sctp/chunk.c +0
-0@@ -264,8 +264,8 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc, frag |=SCTP_DATA_SACK_IMM ; }Quoted 7 lines
net/sctp/chunk.c struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc, · line 268
Quoted 4 lines
net/sctp/chunk.c +2
-2- chunk =sctp_make_datafrag_empty (asoc, sinfo, len, frag, -0 ,GFP_KERNEL ); + chunk = asoc->stream.si->make_datafrag (asoc, sinfo, len, frag, +GFP_KERNEL );I know that none of the sctp code is very optimised, but that indirect call is going to be horrid.
Yeah.. but there is no way to avoid the double derreference considering we only have the asoc pointer in there and we have to reach the contents of the data chunk operations struct, and the .si part is the same as 'stream' part as it's a constant offset.
Quoted 7 lines
Due to the
for()in there, we could add a variable to storeasoc->stream.sioutside the for and then we can do only a single deref inside it. Xin, can you please try and see if the generated code is different?Other suggestions?
Is it worth replacing the si struct with an index/enum value, and indexing an array of method pointer structs? That would save you at least one dereference.
Alternatively you could preform the dereference in two steps (i.e. declare an si pointer on the stack and set it equal to asoc->stream.si, then deref si->make_datafrag at call time. That will at least give the compiler an opportunity to preload the first pointer.
Neil
Quoted 6 lines
Marcelo
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Marcelo Ricardo Leitner
maintainerRe: [PATCHv2 net-next 04/12] sctp: implement make_datafrag for sctp_stream_interleave
Quoted 27 lines · Neil Horman
Quoted 26 lines · Marcelo Ricardo Leitner
Quoted 7 lines · David Laight
From: Xin Long
Quoted 1 line
Sent: 08 December 2017 13:04
...
Quoted 4 lines
net/sctp/chunk.c +0
-0@@ -264,8 +264,8 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc, frag |=SCTP_DATA_SACK_IMM ; }Quoted 7 lines
net/sctp/chunk.c struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc, · line 268
Quoted 4 lines
net/sctp/chunk.c +2
-2- chunk =sctp_make_datafrag_empty (asoc, sinfo, len, frag, -0 ,GFP_KERNEL ); + chunk = asoc->stream.si->make_datafrag (asoc, sinfo, len, frag, +GFP_KERNEL );I know that none of the sctp code is very optimised, but that indirect call is going to be horrid.
Yeah.. but there is no way to avoid the double derreference considering we only have the asoc pointer in there and we have to reach the contents of the data chunk operations struct, and the .si part is the same as 'stream' part as it's a constant offset.
Due to the
for()in there, we could add a variable to storeasoc->stream.sioutside the for and then we can do only a single deref inside it. Xin, can you please try and see if the generated code is different?
Quoted 4 lines
Quoted 2 lines
Other suggestions?
Is it worth replacing the si struct with an index/enum value, and indexing an array of method pointer structs? That would save you at least one dereference.
Hmmm, maybe, yes. It would be like sctp_stream_interleave[asoc->stream.si].make_datafrag(...)
Then same goes for pf->af, probably.
Quoted 5 lines
Alternatively you could preform the dereference in two steps (i.e. declare an si pointer on the stack and set it equal to
asoc->stream.si, then derefsi->make_datafragat call time. That will at least give the compiler an opportunity to preload the first pointer.
Yep, that was my 2nd paragraph above :-) but it only works for cases such as this one.
Marcelo
Quoted 14 lines
Neil
Quoted 6 lines
Marcelo
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv2 net-next 04/12] sctp: implement make_datafrag for sctp_stream_interleave
From: Marcelo Ricardo Leitner
Quoted 1 line
Sent: 08 December 2017 16:00
...
Quoted 5 lines
Quoted 2 lines
Is it worth replacing the si struct with an index/enum value, and indexing an array of method pointer structs? That would save you at least one dereference.
Hmmm, maybe, yes. It would be like sctp_stream_interleave[asoc->stream.si].make_datafrag(...)
If you only expect 2 choices then an if () is likely to produce better code that the above.
The actual implementation can be hidden inside a #define or static inline function.
David
Re: [PATCHv2 net-next 04/12] sctp: implement make_datafrag for sctp_stream_interleave
Quoted 6 lines · David Laight
From: Marcelo Ricardo Leitner
Quoted 1 line
Sent: 08 December 2017 16:00
...
Quoted 3 lines
Quoted 2 lines
Is it worth replacing the si struct with an index/enum value, and indexing an array of method pointer structs? That would save you at least one dereference.
Quoted 9 lines
Quoted 2 lines
Hmmm, maybe, yes. It would be like sctp_stream_interleave[asoc->stream.si].make_datafrag(...)
If you only expect 2 choices then an if () is likely to produce better code that the above.
The actual implementation can be hidden inside a #define or static inline function.
Thats the real question though, will we expect more than two interleaving strategies? Currently its a boolean operation so the answer seems like yes, but is there a possiblity of a biased interleaving, or other worthwhile algorithm?
Neil
Quoted 7 lines
David
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
'Marcelo Ricardo Leitner'
maintainerRe: [PATCHv2 net-next 04/12] sctp: implement make_datafrag for sctp_stream_interleave
Quoted 13 lines · Neil Horman
Quoted 12 lines · David Laight
From: Marcelo Ricardo Leitner
Quoted 1 line
Sent: 08 December 2017 16:00
...
Quoted 5 lines
Quoted 2 lines
Is it worth replacing the si struct with an index/enum value, and indexing an array of method pointer structs? That would save you at least one dereference.
Hmmm, maybe, yes. It would be like sctp_stream_interleave[asoc->stream.si].make_datafrag(...)
If you only expect 2 choices then an if () is likely to produce better code that the above.
Quoted 6 lines
Quoted 3 lines
The actual implementation can be hidden inside a #define or static inline function.
Thats the real question though, will we expect more than two interleaving strategies? Currently its a boolean operation so the answer seems like yes, but is there a possiblity of a biased interleaving, or other worthwhile algorithm?
For the chunk format, I don't think so. It would require another RFC update.
For other possibilities on having a 3rd choice in there, I also don't think so. Stream scheduling is handled apart from it and rx buffer stuff is being covered by it now, don't see how we could have a 3rd option without another chunk format.
But that said, I don't think the macro or even inline wrappers are as clear as the struct with function pointers here and in some cases it even won't avoid the second deref. I wouldn't like to compromise code readability and OO because of 1 fetch, specially considering that this will be barely noticeable.
Neil's idea on using the array of structs and indexing on it is nice. Saves a deref and keeps the abstractions without inserting too much noise on it. Plus, it also allows replacing the struct pointer in sctp_stream with a bit. If then placed before the union in there, we can save up to the whole pointer. Looks like a good compromise to me.
Marcelo
Xin Long
maintainerRe: [PATCHv2 net-next 04/12] sctp: implement make_datafrag for sctp_stream_interleave
In reply to Marcelo Ricardo LeitnerQuoted 39 lines · Marcelo Ricardo Leitner
Quoted 27 lines · Neil Horman
Quoted 26 lines · Marcelo Ricardo Leitner
Quoted 7 lines · David Laight
From: Xin Long
Quoted 1 line
Sent: 08 December 2017 13:04
...
Quoted 4 lines
net/sctp/chunk.c +0
-0@@ -264,8 +264,8 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc, frag |=SCTP_DATA_SACK_IMM ; }Quoted 7 lines
net/sctp/chunk.c struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc, · line 268
Quoted 4 lines
net/sctp/chunk.c +2
-2- chunk =sctp_make_datafrag_empty (asoc, sinfo, len, frag, -0 ,GFP_KERNEL ); + chunk = asoc->stream.si->make_datafrag (asoc, sinfo, len, frag, +GFP_KERNEL );I know that none of the sctp code is very optimised, but that indirect call is going to be horrid.
Yeah.. but there is no way to avoid the double derreference considering we only have the asoc pointer in there and we have to reach the contents of the data chunk operations struct, and the .si part is the same as 'stream' part as it's a constant offset.
Due to the
for()in there, we could add a variable to storeasoc->stream.sioutside the for and then we can do only a single deref inside it. Xin, can you please try and see if the generated code is different?Quoted 4 lines
Quoted 2 lines
Other suggestions?
Is it worth replacing the si struct with an index/enum value, and indexing an array of method pointer structs? That would save you at least one dereference.
Hmmm, maybe, yes. It would be like sctp_stream_interleave[asoc->stream.si].make_datafrag(...)
Then same goes for
pf->af, probably.Quoted 1 line
Quoted 7 lines
Quoted 4 lines
Alternatively you could preform the dereference in two steps (i.e. declare an si pointer on the stack and set it equal to
asoc->stream.si, then derefsi->make_datafragat call time. That will at least give the compiler an opportunity to preload the first pointer.Yep, that was my 2nd paragraph above :-) but it only works for cases such as this one.
Now:
for(N) {
...
chunk = asoc->stream.si->make_datafrag(asoc, sinfo, len, frag,
0x000000000000fb58 <+360>: mov 0x848(%r13),%rax <---- [a]
0x000000000000fb5f <+367>: movzbl %cl,%ecx
0x000000000000fb62 <+370>: mov $0x14000c0,%r8d
0x000000000000fb68 <+376>: mov %r12d,%edx
0x000000000000fb6b <+379>: mov (%rsp),%rsi
0x000000000000fb6f <+383>: mov %r13,%rdi <=(X)
0x000000000000fb72 <+386>: callq *0x8(%rax) <---- [b]
0x000000000000fb78 <+392>: mov %rax,%r15}
ret = N * ([a] + [b])
After using a variable:
struct sctp_stream_interleave *si;
...
si = asoc->stream.si;
0x000000000000fb44 <+340>: mov 0x848(%r14),%rax
0x000000000000fb4e <+350>: mov %rax,0x20(%rsp) <----- [1] for(N) {
...
chunk = si->make_datafrag(asoc, sinfo, len, frag, GFP_KERNEL);
0x000000000000fb69 <+377>: mov 0x20(%rsp),%rax <----- [2]
0x000000000000fb6e <+382>: movzbl %cl,%ecx
0x000000000000fb71 <+385>: mov $0x14000c0,%r8d
0x000000000000fb77 <+391>: mov %r12d,%edx
0x000000000000fb7a <+394>: mov (%rsp),%rsi
0x000000000000fb7e <+398>: mov 0x28(%rsp),%rdi <=(Y)
0x000000000000fb83 <+403>: callq *0x8(%rax) <----- [3]
0x000000000000fb89 <+409>: mov %rax,%r14
}ret = [1] + N * ([2] + [3])
Another small difference:
as you can see, comparing to (X), (Y) is using 0x28(%rsp) in the loop, instead of %r13.
So that's what I can see from the related generated code. If 0x848(%r13) is not worse than 0x28(%rsp) for cpu, I think asoc->stream.si->make_datafrag() is even better. No ?
RE: [PATCHv2 net-next 04/12] sctp: implement make_datafrag for sctp_stream_interleave
From: Xin Long
Quoted 2 lines
Sent: 08 December 2017 16:18
...
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Alternatively you could preform the dereference in two steps (i.e. declare an si pointer on the stack and set it equal to
asoc->stream.si, then derefsi->make_datafragat call time. That will at least give the compiler an opportunity to preload the first pointer.
You want to save the function pointer itself.
...
Quoted 7 lines
Another small difference:
as you can see, comparing to (X), (Y) is using 0x28(%rsp) in the loop, instead of %r13.So that's what I can see from the related generated code. If 0x848(%r13) is not worse than 0x28(%rsp) for cpu, I think
asoc->stream.si->make_datafrag()is even better. No ?
That code must have far too many life local variables. Otherwise there's be a caller saved register available.
David
Xin Long
maintainerRe: [PATCHv2 net-next 04/12] sctp: implement make_datafrag for sctp_stream_interleave
Quoted 16 lines · David Laight
From: Xin Long
Quoted 2 lines
Sent: 08 December 2017 16:18
...
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Alternatively you could preform the dereference in two steps (i.e. declare an si pointer on the stack and set it equal to
asoc->stream.si, then derefsi->make_datafragat call time. That will at least give the compiler an opportunity to preload the first pointer.You want to save the function pointer itself.
...
Quoted 4 lines
Another small difference:
as you can see, comparing to (X), (Y) is using 0x28(%rsp) in the loop, instead of %r13.
Quoted 7 lines
Quoted 3 lines
So that's what I can see from the related generated code. If 0x848(%r13) is not worse than 0x28(%rsp) for cpu, I think
asoc->stream.si->make_datafrag()is even better. No ?That code must have far too many life local variables. Otherwise there's be a caller saved register available.
Hi, David, Sorry, I'm not sure we're worrying about the cpu cost or codes style now ?
For cpu cost, I think 0x848(%r13) operation must be better than the generated code of if-else.
For the codes style, comparing to the if-else, I think this one is more readable. (ignore extendible stuff first, as probably no more new type of data chunk).
RE: [PATCHv2 net-next 04/12] sctp: implement make_datafrag for sctp_stream_interleave
From: Xin Long ...
Quoted 5 lines
Hi, David, Sorry, I'm not sure we're worrying about the cpu cost or codes style now ?
For cpu cost, I think 0x848(%r13) operation must be better than the generated code of if-else.
Nope - the call xxx(%ryyy) is likely to be a data cache miss and a complete cpu pipeline stall.
The conditional will be a data cache hit and the code (for one branch) will be prefetched and speculatively executed.
Some very modern cpu might manage to predict indirect jumps, but for most it is a full pipeline stall.
David
Xin Long
maintainerRe: [PATCHv2 net-next 04/12] sctp: implement make_datafrag for sctp_stream_interleave
Quoted 7 lines · David Laight
From: Xin Long ...
Quoted 5 lines
Hi, David, Sorry, I'm not sure we're worrying about the cpu cost or codes style now ?
For cpu cost, I think 0x848(%r13) operation must be better than the generated code of if-else.
Quoted 8 lines
Nope - the call xxx(%ryyy) is likely to be a data cache miss and a complete cpu pipeline stall.
The conditional will be a data cache hit and the code (for one branch) will be prefetched and speculatively executed.
Some very modern cpu might manage to predict indirect jumps, but for most it is a full pipeline stall.
Thanks for the CPU information.
The thing is with if-else can't avoid xxx(%ryyy) in this case, as Marcelo said above. It seems if-else will just be a extra cost compare to this one.
David Miller
maintainerFrom: Xin Long <[email protected]>
Date: Fri, 8 Dec 2017 21:03:57 +0800
Quoted 4 lines
Stream Interleave would be Implemented in two Parts:
1. The I-DATA Chunk Supporting User Message Interleaving
2. Interaction with Other SCTP Extensions
...
Quoted 16 lines
As the 1st part of Stream Interleave Implementation, this patchset adds an ops framework named
sctp_stream_interleavewith a bunch of stuff that does lots of things needed somewhere.Then it defines
sctp_stream_interleave_0to work for normal DATA chunks andsctp_stream_interleave_1for I-DATA chunks.With these functions, hundreds of if-else checks for the different process on I-DATA chunks would be avoided. Besides, very few codes could be shared in these two function sets.
In this patchset, it adds some basic variables, structures and socket options firstly, then implement these functions one by one to add the procedures for ordered idata gradually, at last adjusts some codes to make them work for unordered idata.
Quoted 7 lines
To make it safe to be implemented and also not break the normal data chunk process, this feature can't be enabled to use until all stream interleave codes are completely accomplished.
v1 -> v2:
- fixed a checkpatch warning that a blank line was missed.
- avoided a kbuild warning reported from gcc-4.9.
Series applied, thanks Xin.