From: Xin Long <lucien.xin@gmail.com> Date: 2017-12-08 13:04:18
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
--
2.1.0
From: Xin Long <lucien.xin@gmail.com> Date: 2017-12-08 13:04:26
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.
Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
---
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(-)
@@ -122,9 +122,12 @@ struct netns_sctp {/* Flag to indicate if PR-CONFIG is enabled. */intreconf_enable;-/* Flag to idicate if SCTP-AUTH is enabled */+/* Flag to indicate if SCTP-AUTH is enabled */intauth_enable;+/* Flag to indicate if stream interleave is enabled */+intintl_enable;+/**PolicytocontrolSCTPIPv4addressscoping*0-DisableIPv4addressscoping
@@ -1940,6 +1941,7 @@ struct sctp_association {__u8need_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;
@@ -3350,7 +3350,10 @@ static int sctp_setsockopt_fragment_interleave(struct sock *sk,if(get_user(val,(int__user*)optval))return-EFAULT;-sctp_sk(sk)->frag_interleave=(val==0)?0:1;+sctp_sk(sk)->frag_interleave=!!val;++if(!sctp_sk(sk)->frag_interleave)+sctp_sk(sk)->strm_interleave=0;return0;}
@@ -4019,6 +4022,40 @@ static int sctp_setsockopt_scheduler_value(struct sock *sk,returnretval;}+staticintsctp_setsockopt_interleaving_supported(structsock*sk,+char__user*optval,+unsignedintoptlen)+{+structsctp_sock*sp=sctp_sk(sk);+structnet*net=sock_net(sk);+structsctp_assoc_valueparams;+intretval=-EINVAL;++if(optlen<sizeof(params))+gotoout;++optlen=sizeof(params);+if(copy_from_user(¶ms,optval,optlen)){+retval=-EFAULT;+gotoout;+}++if(params.assoc_id)+gotoout;++if(!net->sctp.intl_enable||!sp->frag_interleave){+retval=-EPERM;+gotoout;+}++sp->strm_interleave=!!params.assoc_value;++retval=0;++out:+returnretval;+}+/* API 6.2 setsockopt(), getsockopt()**Applicationsusesetsockopt()andgetsockopt()tosetorretrieve
@@ -4206,6 +4243,10 @@ static int sctp_setsockopt(struct sock *sk, int level, int optname,caseSCTP_STREAM_SCHEDULER_VALUE:retval=sctp_setsockopt_scheduler_value(sk,optval,optlen);break;+caseSCTP_INTERLEAVING_SUPPORTED:+retval=sctp_setsockopt_interleaving_supported(sk,optval,+optlen);+break;default:retval=-ENOPROTOOPT;break;
@@ -6981,6 +7022,47 @@ static int sctp_getsockopt_scheduler_value(struct sock *sk, int len,returnretval;}+staticintsctp_getsockopt_interleaving_supported(structsock*sk,intlen,+char__user*optval,+int__user*optlen)+{+structsctp_assoc_valueparams;+structsctp_association*asoc;+intretval=-EFAULT;++if(len<sizeof(params)){+retval=-EINVAL;+gotoout;+}++len=sizeof(params);+if(copy_from_user(¶ms,optval,len))+gotoout;++asoc=sctp_id2assoc(sk,params.assoc_id);+if(asoc){+params.assoc_value=asoc->intl_enable;+}elseif(!params.assoc_id){+structsctp_sock*sp=sctp_sk(sk);++params.assoc_value=sp->strm_interleave;+}else{+retval=-EINVAL;+gotoout;+}++if(put_user(len,optlen))+gotoout;++if(copy_to_user(optval,¶ms,len))+gotoout;++retval=0;++out:+returnretval;+}+staticintsctp_getsockopt(structsock*sk,intlevel,intoptname,char__user*optval,int__user*optlen){
@@ -7171,6 +7253,10 @@ static int sctp_getsockopt(struct sock *sk, int level, int optname,retval=sctp_getsockopt_scheduler_value(sk,len,optval,optlen);break;+caseSCTP_INTERLEAVING_SUPPORTED:+retval=sctp_getsockopt_interleaving_supported(sk,len,optval,+optlen);+break;default:retval=-ENOPROTOOPT;break;
From: Xin Long <lucien.xin@gmail.com> Date: 2017-12-08 13:04:34
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.
Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
---
include/linux/sctp.h | 3 +++
net/sctp/sm_make_chunk.c | 18 ++++++++++++++++--
2 files changed, 19 insertions(+), 2 deletions(-)
From: Xin Long <lucien.xin@gmail.com> Date: 2017-12-08 13:04:43
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.
Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
---
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(+)
From: Xin Long <lucien.xin@gmail.com> Date: 2017-12-08 13:04:51
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.
Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
---
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
@@ -1389,11 +1390,22 @@ struct sctp_stream {structsctp_stream_out_ext*rr_next;};};+structsctp_stream_interleave*si;};#define SCTP_STREAM_CLOSED 0x00#define SCTP_STREAM_OPEN 0x01+staticinline__u16sctp_datachk_len(conststructsctp_stream*stream)+{+returnstream->si->data_chunk_len;+}++staticinline__u16sctp_datahdr_len(conststructsctp_stream*stream)+{+returnstream->si->data_chunk_len-sizeof(structsctp_chunkhdr);+}+/* SCTP_GET_ASSOC_STATS counters */structsctp_priv_assoc_stats{/* Maximum observed rto in the association during subsequent
@@ -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(structsctphdr)-sizeof(structsctp_data_chunk);+sizeof(structsctphdr)-sctp_datachk_len(&asoc->stream);max_data=SCTP_TRUNC4(max_data);/* If the the peer requested that we authenticate DATA chunks
@@ -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,donotpopulatethedatapayload.*/-structsctp_chunk*sctp_make_datafrag_empty(structsctp_association*asoc,+structsctp_chunk*sctp_make_datafrag_empty(conststructsctp_association*asoc,conststructsctp_sndrcvinfo*sinfo,-intdata_len,__u8flags,__u16ssn,-gfp_tgfp)+intlen,__u8flags,gfp_tgfp){structsctp_chunk*retval;structsctp_datahdrdp;-intchunk_len;/* We assign the TSN as LATE as possible, not here when*creatingthechunk.*/-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)-gotonodata;+returnNULL;retval->subh.data_hdr=sctp_addto_chunk(retval,sizeof(dp),&dp);memcpy(&retval->sinfo,sinfo,sizeof(structsctp_sndrcvinfo));-nodata:returnretval;}
@@ -0,0 +1,79 @@+/* SCTP kernel implementation+*(C)CopyrightRedHatInc.2017+*+*ThisfileispartoftheSCTPkernelimplementation+*+*Thesefunctionsmanipulatesctpstreamqueue/scheduling.+*+*ThisSCTPimplementationisfreesoftware;+*youcanredistributeitand/ormodifyitunderthetermsof+*theGNUGeneralPublicLicenseaspublishedby+*theFreeSoftwareFoundation;eitherversion2,or(atyouroption)+*anylaterversion.+*+*ThisSCTPimplementationisdistributedinthehopethatit+*willbeuseful,butWITHOUTANYWARRANTY;withouteventheimplied+*************************+*warrantyofMERCHANTABILITYorFITNESSFORAPARTICULARPURPOSE.+*SeetheGNUGeneralPublicLicenseformoredetails.+*+*YoushouldhavereceivedacopyoftheGNUGeneralPublicLicense+*alongwithGNUCC;seethefileCOPYING.Ifnot,see+*<http://www.gnu.org/licenses/>.+*+*Pleasesendanybugreportsorfixesyoumaketothe+*emailaddresched(es):+*lksctpdevelopers<linux-sctp@vger.kernel.org>+*+*Writtenormodifiedby:+*XinLong<lucien.xin@gmail.com>+*/++#include<net/sctp/sctp.h>+#include<net/sctp/sm.h>+#include<linux/sctp.h>++staticstructsctp_chunk*sctp_make_idatafrag_empty(+conststructsctp_association*asoc,+conststructsctp_sndrcvinfo*sinfo,+intlen,__u8flags,gfp_tgfp)+{+structsctp_chunk*retval;+structsctp_idatahdrdp;++memset(&dp,0,sizeof(dp));+dp.stream=htons(sinfo->sinfo_stream);++if(sinfo->sinfo_flags&SCTP_UNORDERED)+flags|=SCTP_DATA_UNORDERED;++retval=sctp_make_idata(asoc,flags,sizeof(dp)+len,gfp);+if(!retval)+returnNULL;++retval->subh.idata_hdr=sctp_addto_chunk(retval,sizeof(dp),&dp);+memcpy(&retval->sinfo,sinfo,sizeof(structsctp_sndrcvinfo));++returnretval;+}++staticstructsctp_stream_interleavesctp_stream_interleave_0={+.data_chunk_len=sizeof(structsctp_data_chunk),+/* DATA process functions */+.make_datafrag=sctp_make_datafrag_empty,+};++staticstructsctp_stream_interleavesctp_stream_interleave_1={+.data_chunk_len=sizeof(structsctp_idata_chunk),+/* I-DATA process functions */+.make_datafrag=sctp_make_idatafrag_empty,+};++voidsctp_stream_interleave_init(structsctp_stream*stream)+{+structsctp_association*asoc;++asoc=container_of(stream,structsctp_association,stream);+stream->si=asoc->intl_enable?&sctp_stream_interleave_1+:&sctp_stream_interleave_0;+}
From: Xin Long <lucien.xin@gmail.com> Date: 2017-12-08 13:05:00
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.
Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
---
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(-)
@@ -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)+/**PointerstoaddressrelatedSCTPfunctions.*(i.e.thingsthatdependontheaddressfamily.)
@@ -623,6 +635,7 @@ struct sctp_chunk {__u16rtt_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_ssnsingleton: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? */
@@ -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){caseSCTP_CID_DATA:+caseSCTP_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,*ordelayinhopesofbundlingafullsizedpacket.*/if(chunk->skb->len+q->out_qlen>transport->pathmtu--packet->overhead-sizeof(structsctp_data_chunk)-4)+packet->overhead-sctp_datachk_len(&chunk->asoc->stream)-4)/* Enough data queued to fill a packet */returnSCTP_XMIT_OK;
@@ -2002,7 +2002,20 @@ static int sctp_sendmsg(struct sock *sk, struct msghdr *msg, size_t msg_len)if(err<0)gotoout_free;-wait_connect=true;+/* If stream interleave is enabled, wait_connect has to be+*doneearlierthandataenqueue,asitneedstomakedata+*oridataaccordingtoasoc->intl_enablewhichisset+*afterconnectionisdone.+*/+if(sctp_sk(asoc->base.sk)->strm_interleave){+timeo=sock_sndtimeo(sk,0);+err=sctp_wait_for_connect(asoc,&timeo);+if(err)+gotoout_unlock;+}else{+wait_connect=true;+}+pr_debug("%s: we associated primitively\n",__func__);}
@@ -443,8 +443,8 @@ struct sctp_ulpevent *sctp_ulpevent_make_send_failed(gotofail;/* Pull off the common chunk header and DATA header. */-skb_pull(skb,sizeof(structsctp_data_chunk));-len-=sizeof(structsctp_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);
From: Xin Long <lucien.xin@gmail.com> Date: 2017-12-08 13:05:08
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.
Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
---
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(-)
@@ -6299,7 +6297,7 @@ static int sctp_eat_data(const struct sctp_association *asoc,*Actually,allowalittlebitofoverflow(uptoaMTU).*/datalen=ntohs(chunk->chunk_hdr->length);-datalen-=sizeof(structsctp_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,*withcausesetto"Invalid Stream Identifier"(SeeSection3.3.10)*anddiscardtheDATAchunk.*/-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,*SSNissmallerthenthenextexpectedone.Ifitis,itwrapped*andisinvalid.*/-ssn=ntohs(data_hdr->ssn);-if(ordered&&SSN_lt(ssn,sctp_ssn_peek(&asoc->stream,in,sid)))+if(!asoc->stream.si->validate_data(chunk))returnSCTP_IERROR_PROTO_VIOLATION;/* Send the data up to the user. Note: Schedule the
From: Xin Long <lucien.xin@gmail.com> Date: 2017-12-08 13:05:16
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.
Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
---
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(-)
@@ -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*growingthisstructureasitisatthemaximumlimitnow.+*+*sctp_ulpeventissavedinsk->cb(48bytes),whoselast4bytes+*havebeentakenbysock_skb_cb,Sohereithastouse'packed'+*tomakesctp_ulpeventfitintotherest44bytes.*/structsctp_ulpevent{structsctp_association*asoc;structsctp_chunk*chunk;unsignedintrmem_len;-__u32ppid;+union{+__u32mid;+__u16ssn;+};+union{+__u32ppid;+__u32fsn;+};__u32tsn;__u32cumtsn;__u16stream;-__u16ssn;__u16flags;__u16msg_flags;-};+}__packed;/* Retrieve the skb this event sits inside of. */staticinlinestructsk_buff*sctp_event2skb(conststructsctp_ulpevent*ev)
@@ -853,7 +857,7 @@ static struct sctp_ulpevent *sctp_ulpq_order(struct sctp_ulpq *ulpq,structsctp_stream*stream;/* Check if this message needs ordering. */-if(SCTP_DATA_UNORDERED&event->msg_flags)+if(event->msg_flags&SCTP_DATA_UNORDERED)returnevent;/* Note: The stream ID must be verified before this routine. */
From: Xin Long <lucien.xin@gmail.com> Date: 2017-12-08 13:05:22
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.
Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
---
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(-)
@@ -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. */
From: Xin Long <lucien.xin@gmail.com> Date: 2017-12-08 13:05:30
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.
Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
---
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(-)
@@ -545,6 +545,113 @@ static int sctp_ulpevent_idata(struct sctp_ulpq *ulpq,returnevent_eor;}+staticstructsctp_ulpevent*sctp_intl_retrieve_first(structsctp_ulpq*ulpq)+{+structsctp_stream_in*csin,*sin=NULL;+structsk_buff*first_frag=NULL;+structsk_buff*last_frag=NULL;+structsctp_ulpevent*retval;+structsk_buff*pos;+__u32next_fsn=0;+__u16sid=0;++skb_queue_walk(&ulpq->reasm,pos){+structsctp_ulpevent*cevent=sctp_skb2event(pos);++csin=sctp_stream_in(ulpq->asoc,cevent->stream);+if(csin->pd_mode)+continue;++switch(cevent->msg_flags&SCTP_DATA_FRAG_MASK){+caseSCTP_DATA_FIRST_FRAG:+if(first_frag)+gotoout;+if(cevent->mid==csin->mid){+first_frag=pos;+last_frag=pos;+next_fsn=0;+sin=csin;+sid=cevent->stream;+}+break;+caseSCTP_DATA_MIDDLE_FRAG:+if(!first_frag)+break;+if(cevent->stream==sid&&+cevent->mid==sin->mid&&+cevent->fsn==next_fsn){+next_fsn++;+last_frag=pos;+}else{+gotoout;+}+break;+caseSCTP_DATA_LAST_FRAG:+if(first_frag)+gotoout;+break;+default:+break;+}+}++if(!first_frag)+returnNULL;++out:+retval=sctp_make_reassembled_event(sock_net(ulpq->asoc->base.sk),+&ulpq->reasm,first_frag,+last_frag);+if(retval){+sin->fsn=next_fsn;+sin->pd_mode=1;+}++returnretval;+}++staticvoidsctp_intl_start_pd(structsctp_ulpq*ulpq,gfp_tgfp)+{+structsctp_ulpevent*event;++if(skb_queue_empty(&ulpq->reasm))+return;++do{+event=sctp_intl_retrieve_first(ulpq);+if(event)+sctp_enqueue_event(ulpq,event);+}while(event);+}++staticvoidsctp_renege_events(structsctp_ulpq*ulpq,structsctp_chunk*chunk,+gfp_tgfp)+{+structsctp_association*asoc=ulpq->asoc;+__u32freed=0;+__u16needed;++if(chunk){+needed=ntohs(chunk->chunk_hdr->length);+needed-=sizeof(structsctp_idata_chunk);+}else{+needed=SCTP_DEFAULT_MAXWINDOW;+}++if(skb_queue_empty(&asoc->base.sk->sk_receive_queue)){+freed=sctp_ulpq_renege_list(ulpq,&ulpq->lobby,needed);+if(freed<needed)+freed+=sctp_ulpq_renege_list(ulpq,&ulpq->reasm,+needed);+}++if(chunk&&freed>=needed)+if(sctp_ulpevent_idata(ulpq,chunk,gfp)<=0)+sctp_intl_start_pd(ulpq,gfp);++sk_mem_reclaim(asoc->base.sk);+}+staticstructsctp_stream_interleavesctp_stream_interleave_0={.data_chunk_len=sizeof(structsctp_data_chunk),/* DATA process functions */
From: Xin Long <lucien.xin@gmail.com> Date: 2017-12-08 13:05:39
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.
Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
---
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(-)
From: Xin Long <lucien.xin@gmail.com> Date: 2017-12-08 13:05:47
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.
Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
---
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(-)
@@ -652,6 +652,103 @@ static void sctp_renege_events(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,sk_mem_reclaim(asoc->base.sk);}+staticvoidsctp_intl_stream_abort_pd(structsctp_ulpq*ulpq,__u16sid,+__u32mid,__u16flags,gfp_tgfp)+{+structsock*sk=ulpq->asoc->base.sk;+structsctp_ulpevent*ev=NULL;++if(!sctp_ulpevent_type_enabled(SCTP_PARTIAL_DELIVERY_EVENT,+&sctp_sk(sk)->subscribe))+return;++ev=sctp_ulpevent_make_pdapi(ulpq->asoc,SCTP_PARTIAL_DELIVERY_ABORTED,+sid,mid,flags,gfp);+if(ev){+__skb_queue_tail(&sk->sk_receive_queue,sctp_event2skb(ev));++if(!sctp_sk(sk)->data_ready_signalled){+sctp_sk(sk)->data_ready_signalled=1;+sk->sk_data_ready(sk);+}+}+}++staticvoidsctp_intl_reap_ordered(structsctp_ulpq*ulpq,__u16sid)+{+structsctp_stream*stream=&ulpq->asoc->stream;+structsctp_ulpevent*cevent,*event=NULL;+structsk_buff_head*lobby=&ulpq->lobby;+structsk_buff*pos,*tmp;+structsk_buff_headtemp;+__u16csid;+__u32cmid;++skb_queue_head_init(&temp);+sctp_skb_for_each(pos,lobby,tmp){+cevent=(structsctp_ulpevent*)pos->cb;+csid=cevent->stream;+cmid=cevent->mid;++if(csid>sid)+break;++if(csid<sid)+continue;++if(!MID_lt(cmid,sctp_mid_peek(stream,in,csid)))+break;++__skb_unlink(pos,lobby);+if(!event)+event=sctp_skb2event(pos);++__skb_queue_tail(&temp,pos);+}++if(!event&&pos!=(structsk_buff*)lobby){+cevent=(structsctp_ulpevent*)pos->cb;+csid=cevent->stream;+cmid=cevent->mid;++if(csid==sid&&cmid==sctp_mid_peek(stream,in,csid)){+sctp_mid_next(stream,in,csid);+__skb_unlink(pos,lobby);+__skb_queue_tail(&temp,pos);+event=sctp_skb2event(pos);+}+}++if(event){+sctp_intl_retrieve_ordered(ulpq,event);+sctp_enqueue_event(ulpq,event);+}+}++staticvoidsctp_intl_abort_pd(structsctp_ulpq*ulpq,gfp_tgfp)+{+structsctp_stream*stream=&ulpq->asoc->stream;+__u16sid;++for(sid=0;sid<stream->incnt;sid++){+structsctp_stream_in*sin=&stream->in[sid];+__u32mid;++if(sin->pd_mode){+sin->pd_mode=0;++mid=sin->mid;+sctp_intl_stream_abort_pd(ulpq,sid,mid,0,gfp);+sctp_mid_skip(stream,in,sid,mid);++sctp_intl_reap_ordered(ulpq,sid);+}+}++/* intl abort pd happens only when all data needs to be cleaned */+sctp_ulpq_flush(ulpq);+}+staticstructsctp_stream_interleavesctp_stream_interleave_0={.data_chunk_len=sizeof(structsctp_data_chunk),/* DATA process functions */
From: Xin Long <lucien.xin@gmail.com> Date: 2017-12-08 13:05:55
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.
Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
---
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(-)
@@ -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++)+/**PointerstoaddressrelatedSCTPfunctions.*(i.e.thingsthatdependontheaddressfamily.)
@@ -201,6 +201,22 @@ static void sctp_for_each_tx_datachunk(struct sctp_association *asoc,cb(chunk);}+staticvoidsctp_for_each_rx_skb(structsctp_association*asoc,structsock*sk,+void(*cb)(structsk_buff*,structsock*))++{+structsk_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. */staticinlineintsctp_verify_addr(structsock*sk,unionsctp_addr*addr,intlen)
@@ -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*originalUDP-stylesocketorcreatedwiththeaccept()callona
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
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.
...
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
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.
...
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.
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
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.
...
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.
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.
I was getting two bits mixed up.
But the code that reads data_chunk_len is following an awful lot of pointers.
David
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?
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
Marcelo
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
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.
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.
Yep, that was my 2nd paragraph above :-) but it only works for cases
such as this one.
Marcelo
Neil
quoted
Marcelo
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@vger.kernel.org
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 majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
...
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.
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.
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.
I was getting two bits mixed up.
But the code that reads data_chunk_len is 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
From: David Laight <hidden> Date: 2017-12-08 16:04:43
From: Marcelo Ricardo Leitner
Sent: 08 December 2017 16:00
...
quoted
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
From: Neil Horman <nhorman@tuxdriver.com> Date: 2017-12-08 16:09:45
On Fri, Dec 08, 2017 at 04:04:58PM +0000, David Laight wrote:
From: Marcelo Ricardo Leitner
quoted
Sent: 08 December 2017 16:00
...
quoted
quoted
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.
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
David
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
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
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.
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 ?
From: David Laight <hidden> Date: 2017-12-08 16:21:50
From: Xin Long
Sent: 08 December 2017 16:18
...
quoted
quoted
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.
You want to save the function pointer itself.
...
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
From: Xin Long <lucien.xin@gmail.com> Date: 2017-12-08 17:23:32
On Sat, Dec 9, 2017 at 12:22 AM, David Laight [off-list ref] wrote:
From: Xin Long
quoted
Sent: 08 December 2017 16:18
...
quoted
quoted
quoted
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.
You want to save the function pointer itself.
...
quoted
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.
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).
From: David Laight <hidden> Date: 2017-12-08 17:28:42
From: Xin Long ...
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
From: Xin Long <lucien.xin@gmail.com> Date: 2017-12-08 17:37:15
On Sat, Dec 9, 2017 at 1:29 AM, David Laight [off-list ref] wrote:
From: Xin Long ...
quoted
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.
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.
On Fri, Dec 08, 2017 at 11:08:56AM -0500, Neil Horman wrote:
On Fri, Dec 08, 2017 at 04:04:58PM +0000, David Laight wrote:
quoted
From: Marcelo Ricardo Leitner
quoted
Sent: 08 December 2017 16:00
...
quoted
quoted
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.
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
From: David Miller <davem@davemloft.net> Date: 2017-12-11 16:23:35
From: Xin Long <lucien.xin@gmail.com>
Date: Fri, 8 Dec 2017 21:03:57 +0800
Stream Interleave would be Implemented in two Parts:
1. The I-DATA Chunk Supporting User Message Interleaving
2. Interaction with Other SCTP Extensions
...
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.