| Message ID | 0d8d8e5482205ead64a2cfd2d414e918ce5602dd.1484845510.git.lucien.xin@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Xin Long > Sent: 19 January 2017 17:19 > This patch is to define Add Incoming/Outgoing Streams Request > Parameter described in rfc6525 section 4.5 and 4.6. They can > be in one same chunk trunk as rfc6525 section 3.1-7 describes, > so make them in one function. ... > +struct sctp_strreset_addstrm { > + sctp_paramhdr_t param_hdr; > + __u32 request_seq; > + __u16 number_of_streams; > + __u16 reserved; > +} __packed; ... > + addstrm.param_hdr.type = SCTP_PARAM_RESET_ADD_OUT_STREAMS; > + addstrm.param_hdr.length = htons(size); > + addstrm.number_of_streams = htons(out); > + addstrm.request_seq = htonl(asoc->strreset_outseq); > + addstrm.reserved = 0; > + > + sctp_addto_chunk(retval, size, &addstrm); Since you allocate the sctp_strreset_addstrm structure on stack there is no requirement for it to be packed. David
On Fri, Jan 20, 2017 at 02:50:01PM +0000, David Laight wrote: > From: Xin Long > > Sent: 19 January 2017 17:19 > > This patch is to define Add Incoming/Outgoing Streams Request > > Parameter described in rfc6525 section 4.5 and 4.6. They can > > be in one same chunk trunk as rfc6525 section 3.1-7 describes, > > so make them in one function. > ... > > +struct sctp_strreset_addstrm { > > + sctp_paramhdr_t param_hdr; > > + __u32 request_seq; > > + __u16 number_of_streams; > > + __u16 reserved; > > +} __packed; > ... > > + addstrm.param_hdr.type = SCTP_PARAM_RESET_ADD_OUT_STREAMS; > > + addstrm.param_hdr.length = htons(size); > > + addstrm.number_of_streams = htons(out); > > + addstrm.request_seq = htonl(asoc->strreset_outseq); > > + addstrm.reserved = 0; > > + > > + sctp_addto_chunk(retval, size, &addstrm); > > Since you allocate the sctp_strreset_addstrm structure on stack > there is no requirement for it to be packed. It shouldn't matter that it's allocated on stack. Why should it? We need it to be packed as this is a header that will be sent out to another peer, so there can't be any padding on it. Marcelo
From: Marcelo Ricardo Leitner > Sent: 20 January 2017 16:39 > To: David Laight > On Fri, Jan 20, 2017 at 02:50:01PM +0000, David Laight wrote: > > From: Xin Long > > > Sent: 19 January 2017 17:19 > > > This patch is to define Add Incoming/Outgoing Streams Request > > > Parameter described in rfc6525 section 4.5 and 4.6. They can > > > be in one same chunk trunk as rfc6525 section 3.1-7 describes, > > > so make them in one function. > > ... > > > +struct sctp_strreset_addstrm { > > > + sctp_paramhdr_t param_hdr; > > > + __u32 request_seq; > > > + __u16 number_of_streams; > > > + __u16 reserved; > > > +} __packed; > > ... > > > + addstrm.param_hdr.type = SCTP_PARAM_RESET_ADD_OUT_STREAMS; > > > + addstrm.param_hdr.length = htons(size); > > > + addstrm.number_of_streams = htons(out); > > > + addstrm.request_seq = htonl(asoc->strreset_outseq); > > > + addstrm.reserved = 0; > > > + > > > + sctp_addto_chunk(retval, size, &addstrm); > > > > Since you allocate the sctp_strreset_addstrm structure on stack > > there is no requirement for it to be packed. > > It shouldn't matter that it's allocated on stack. Why should it? > We need it to be packed as this is a header that will be sent out to > another peer, so there can't be any padding on it. That isn't what __packed means. It means that the compiler must assume that the structure can be misaligned in memory and must use byte memory accesses on systems that fault misaligned memory accesses. David
On Mon, Jan 23, 2017 at 12:26:12PM +0000, David Laight wrote: > From: Marcelo Ricardo Leitner > Sent: 20 January 2017 16:39 > > To: David Laight > > On Fri, Jan 20, 2017 at 02:50:01PM +0000, David Laight wrote: > > > From: Xin Long > > > > Sent: 19 January 2017 17:19 > > > > This patch is to define Add Incoming/Outgoing Streams Request > > > > Parameter described in rfc6525 section 4.5 and 4.6. They can > > > > be in one same chunk trunk as rfc6525 section 3.1-7 describes, > > > > so make them in one function. > > > ... > > > > +struct sctp_strreset_addstrm { > > > > + sctp_paramhdr_t param_hdr; > > > > + __u32 request_seq; > > > > + __u16 number_of_streams; > > > > + __u16 reserved; > > > > +} __packed; > > > ... > > > > + addstrm.param_hdr.type = SCTP_PARAM_RESET_ADD_OUT_STREAMS; > > > > + addstrm.param_hdr.length = htons(size); > > > > + addstrm.number_of_streams = htons(out); > > > > + addstrm.request_seq = htonl(asoc->strreset_outseq); > > > > + addstrm.reserved = 0; > > > > + > > > > + sctp_addto_chunk(retval, size, &addstrm); > > > > > > Since you allocate the sctp_strreset_addstrm structure on stack > > > there is no requirement for it to be packed. > > > > It shouldn't matter that it's allocated on stack. Why should it? > > We need it to be packed as this is a header that will be sent out to > > another peer, so there can't be any padding on it. > > That isn't what __packed means. > It means that the compiler must assume that the structure can be > misaligned in memory and must use byte memory accesses on systems > that fault misaligned memory accesses. That's a side-effect of it. https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html#Common-Type-Attributes "This attribute, attached to struct or union type definition, specifies that each member (other than zero-width bit-fields) of the structure or union is placed to minimize the memory required. " So, no padding. A field just after the other, which is what we want on a network header. Marcelo
From: David Laight <David.Laight@ACULAB.COM> Date: Mon, 23 Jan 2017 12:26:12 +0000 > From: Marcelo Ricardo Leitner > Sent: 20 January 2017 16:39 >> To: David Laight >> On Fri, Jan 20, 2017 at 02:50:01PM +0000, David Laight wrote: >> > From: Xin Long >> > > Sent: 19 January 2017 17:19 >> > > This patch is to define Add Incoming/Outgoing Streams Request >> > > Parameter described in rfc6525 section 4.5 and 4.6. They can >> > > be in one same chunk trunk as rfc6525 section 3.1-7 describes, >> > > so make them in one function. >> > ... >> > > +struct sctp_strreset_addstrm { >> > > + sctp_paramhdr_t param_hdr; >> > > + __u32 request_seq; >> > > + __u16 number_of_streams; >> > > + __u16 reserved; >> > > +} __packed; >> > ... >> > > + addstrm.param_hdr.type = SCTP_PARAM_RESET_ADD_OUT_STREAMS; >> > > + addstrm.param_hdr.length = htons(size); >> > > + addstrm.number_of_streams = htons(out); >> > > + addstrm.request_seq = htonl(asoc->strreset_outseq); >> > > + addstrm.reserved = 0; >> > > + >> > > + sctp_addto_chunk(retval, size, &addstrm); >> > >> > Since you allocate the sctp_strreset_addstrm structure on stack >> > there is no requirement for it to be packed. >> >> It shouldn't matter that it's allocated on stack. Why should it? >> We need it to be packed as this is a header that will be sent out to >> another peer, so there can't be any padding on it. > > That isn't what __packed means. > It means that the compiler must assume that the structure can be > misaligned in memory and must use byte memory accesses on systems > that fault misaligned memory accesses. Also, for the types involved there will not be any padding at all. Check if you do not believe me. If this is "so critical" for end to end communication, why the heck do you not see __packed sprinkled all over our definitions for IPV4, IPV6, TCP, UDP, etc. headers? Do you know why? Because it's completely unnecessary... Can I seriously, strongly, state that people should use __packed as the last possible resort when writing code? Don't add it unless you run into a problem which fundamentally cannot be solved by either using different data types, a different ordering of data items, or similar. And when __packed is actually legitimate and used I am now going to require a HUGE FRIGGIN' COMMENT explaining why no other approach whatsoever can solve the problem. __packed is going to emit incredibly inefficient code on some cpus, I really don't think people truly understand the enormous negative ramifications of using __packed. It is almost never, ever, necessary. And I'm tired of writing this tirade every half year or so. __packed, just say no...
From: "'Marcelo Ricardo Leitner'" <marcelo.leitner@gmail.com> Date: Mon, 23 Jan 2017 10:36:28 -0200 > So, no padding. A field just after the other, which is what we want on a > network header. It isn't necessary! Show me a case where it is required when you use properly fixed sized types and a proper ordering of the struct members. No padding is going in there, go and check. Do we splatter __packed all over our ipv4/ipv6 header, TCP header, UDP header, etc. structures? No, we don't because it's totally unecessary. I will not accept __packed being used unless it is absolutely, provably, the only way to solve a particular problem. And when that does happen, I am going to require a huge comment explaining in detail why this is the case, and why no other approach or solution solved the problem.
On Mon, Jan 23, 2017 at 11:00:47AM -0500, David Miller wrote: > From: "'Marcelo Ricardo Leitner'" <marcelo.leitner@gmail.com> > Date: Mon, 23 Jan 2017 10:36:28 -0200 > > > So, no padding. A field just after the other, which is what we want on a > > network header. > > It isn't necessary! > > Show me a case where it is required when you use properly fixed sized > types and a proper ordering of the struct members. No padding is > going in there, go and check. > > Do we splatter __packed all over our ipv4/ipv6 header, TCP header, UDP > header, etc. structures? No, we don't because it's totally unecessary. Err, sure, right. > > I will not accept __packed being used unless it is absolutely, provably, > the only way to solve a particular problem. And when that does happen, > I am going to require a huge comment explaining in detail why this is > the case, and why no other approach or solution solved the problem. Would this be a candidate for checkpatch.pl? Marcelo
From: marcelo.leitner@gmail.com Date: Mon, 23 Jan 2017 14:14:58 -0200 > On Mon, Jan 23, 2017 at 11:00:47AM -0500, David Miller wrote: >> I will not accept __packed being used unless it is absolutely, provably, >> the only way to solve a particular problem. And when that does happen, >> I am going to require a huge comment explaining in detail why this is >> the case, and why no other approach or solution solved the problem. > > Would this be a candidate for checkpatch.pl? If this helps developers, sure.
On Mon, Jan 23, 2017 at 10:58:10AM -0500, David Miller wrote: > If this is "so critical" for end to end communication, why the heck > do you not see __packed sprinkled all over our definitions for IPV4, > IPV6, TCP, UDP, etc. headers? > > Do you know why? Because it's completely unnecessary... Btw, virtually all sctp headers are currently tagged with __packed, since forever. $ git grep __packed -- include/linux/sctp.h include/linux/sctp.h:} __packed sctp_sctphdr_t; include/linux/sctp.h:} __packed sctp_chunkhdr_t; include/linux/sctp.h:} __packed sctp_paramhdr_t; include/linux/sctp.h:} __packed sctp_datahdr_t; include/linux/sctp.h:} __packed sctp_data_chunk_t; include/linux/sctp.h:} __packed sctp_inithdr_t; include/linux/sctp.h:} __packed sctp_init_chunk_t; include/linux/sctp.h:} __packed sctp_ipv4addr_param_t; include/linux/sctp.h:} __packed sctp_ipv6addr_param_t; include/linux/sctp.h:} __packed sctp_cookie_preserve_param_t; include/linux/sctp.h:} __packed sctp_hostname_param_t; include/linux/sctp.h:} __packed sctp_supported_addrs_param_t; include/linux/sctp.h:} __packed sctp_ecn_capable_param_t; include/linux/sctp.h:} __packed sctp_adaptation_ind_param_t; include/linux/sctp.h:} __packed sctp_supported_ext_param_t; include/linux/sctp.h:} __packed sctp_random_param_t; include/linux/sctp.h:} __packed sctp_chunks_param_t; include/linux/sctp.h:} __packed sctp_hmac_algo_param_t; include/linux/sctp.h:} __packed sctp_cookie_param_t; include/linux/sctp.h:} __packed sctp_unrecognized_param_t; include/linux/sctp.h:} __packed sctp_gap_ack_block_t; include/linux/sctp.h:} __packed sctp_sackhdr_t; include/linux/sctp.h:} __packed sctp_sack_chunk_t; include/linux/sctp.h:} __packed sctp_heartbeathdr_t; include/linux/sctp.h:} __packed sctp_heartbeat_chunk_t; include/linux/sctp.h:} __packed sctp_abort_chunk_t; include/linux/sctp.h:} __packed sctp_shutdownhdr_t; include/linux/sctp.h:} __packed; include/linux/sctp.h:} __packed sctp_errhdr_t; include/linux/sctp.h:} __packed sctp_operr_chunk_t; include/linux/sctp.h:} __packed sctp_ecne_chunk_t; include/linux/sctp.h:} __packed sctp_cwr_chunk_t; ... ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 64) } __attribute__((packed)) sctp_sctphdr_t; I'm reviewing them all and will probably post a patch to remove them. Marcelo
From: marcelo.leitner@gmail.com Date: Sun, 29 Jan 2017 12:31:17 -0200 > On Mon, Jan 23, 2017 at 10:58:10AM -0500, David Miller wrote: >> If this is "so critical" for end to end communication, why the heck >> do you not see __packed sprinkled all over our definitions for IPV4, >> IPV6, TCP, UDP, etc. headers? >> >> Do you know why? Because it's completely unnecessary... > > Btw, virtually all sctp headers are currently tagged with __packed, > since forever. ... > I'm reviewing them all and will probably post a patch to remove them. Thanks.
diff --git a/include/linux/sctp.h b/include/linux/sctp.h index 95b8ed3..f1f494f 100644 --- a/include/linux/sctp.h +++ b/include/linux/sctp.h @@ -742,4 +742,11 @@ struct sctp_strreset_tsnreq { __u32 request_seq; } __packed; +struct sctp_strreset_addstrm { + sctp_paramhdr_t param_hdr; + __u32 request_seq; + __u16 number_of_streams; + __u16 reserved; +} __packed; + #endif /* __LINUX_SCTP_H__ */ diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h index ac37c17..3675fde 100644 --- a/include/net/sctp/sm.h +++ b/include/net/sctp/sm.h @@ -267,6 +267,9 @@ struct sctp_chunk *sctp_make_strreset_req( bool out, bool in); struct sctp_chunk *sctp_make_strreset_tsnreq( const struct sctp_association *asoc); +struct sctp_chunk *sctp_make_strreset_addstrm( + const struct sctp_association *asoc, + __u16 out, __u16 in); void sctp_chunk_assign_tsn(struct sctp_chunk *); void sctp_chunk_assign_ssn(struct sctp_chunk *); diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c index 801450c..a44546d 100644 --- a/net/sctp/sm_make_chunk.c +++ b/net/sctp/sm_make_chunk.c @@ -3689,3 +3689,49 @@ struct sctp_chunk *sctp_make_strreset_tsnreq( return retval; } + +/* RE-CONFIG 4.5/4.6 (ADD STREAM) + * 0 1 2 3 + * 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ + * | Parameter Type = 17 | Parameter Length = 12 | + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ + * | Re-configuration Request Sequence Number | + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ + * | Number of new streams | Reserved | + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ + */ +struct sctp_chunk *sctp_make_strreset_addstrm( + const struct sctp_association *asoc, + __u16 out, __u16 in) +{ + struct sctp_strreset_addstrm addstrm; + __u16 size = sizeof(addstrm); + struct sctp_chunk *retval; + + retval = sctp_make_reconf(asoc, (!!out + !!in) * size); + if (!retval) + return NULL; + + if (out) { + addstrm.param_hdr.type = SCTP_PARAM_RESET_ADD_OUT_STREAMS; + addstrm.param_hdr.length = htons(size); + addstrm.number_of_streams = htons(out); + addstrm.request_seq = htonl(asoc->strreset_outseq); + addstrm.reserved = 0; + + sctp_addto_chunk(retval, size, &addstrm); + } + + if (in) { + addstrm.param_hdr.type = SCTP_PARAM_RESET_ADD_IN_STREAMS; + addstrm.param_hdr.length = htons(size); + addstrm.number_of_streams = htons(in); + addstrm.request_seq = htonl(asoc->strreset_outseq + !!out); + addstrm.reserved = 0; + + sctp_addto_chunk(retval, size, &addstrm); + } + + return retval; +}
This patch is to define Add Incoming/Outgoing Streams Request Parameter described in rfc6525 section 4.5 and 4.6. They can be in one same chunk trunk as rfc6525 section 3.1-7 describes, so make them in one function. Signed-off-by: Xin Long <lucien.xin@gmail.com> --- include/linux/sctp.h | 7 +++++++ include/net/sctp/sm.h | 3 +++ net/sctp/sm_make_chunk.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+)