sctp: remove unnecessary byteshifting, calculate crc in big-endian

PATCH Superseded Harvey Harrison 2 participants 8 messages Jul 4, 2008 – Jul 17, 2008

Subsystems (4)

Networking Network drivers netfilter SCTP protocol

lore

Share & tools

lore

Harvey Harrison

author

2008-07-04 01:13 UTC #0 lore raw

PATCH 1/1 sctp: remove unnecessary byteshifting, calculate crc in big-endian

Signed-off-by Harvey Harrison <[email protected]>

---
 include/net/sctp/checksum.h            |   23 ++++++++++++++---------
 net/ipv4/netfilter/nf_nat_proto_sctp.c |    4 ++--
 net/sctp/input.c                       |    4 ++--
 net/sctp/output.c                      |    4 ++--
 4 files changed, 20 insertions(+), 15 deletions(-)

include/net/sctp/checksum.h +14 -9

index ba75c67..b799fb2 100644
@@ -46,9 +46,14 @@
 #include <net/sctp/sctp.h>
 #include <linux/crc32c.h>
 
-static inline __u32 sctp_start_cksum(__u8 *buffer, __u16 length)
+static inline __be32 sctp_crc32c(__be32 crc, u8 *buffer, u16 length)
 {
-	__u32 crc = ~(__u32) 0;
+	return (__force __be32)crc32c((__force u32)crc, buffer, length);
+}
+
+static inline __be32 sctp_start_cksum(__u8 *buffer, __u16 length)
+{
+	__be32 crc = ~cpu_to_be32(0);
 	__u8  zero[sizeof(__u32)] = {0};
 
 	/* Optimize this routine to be SCTP specific, knowing how

@@ -56,23 +61,23 @@ static inline __u32 sctp_start_cksum(__u8 *buffer, __u16 length)
 	 */
 
 	/* Calculate CRC up to the checksum. */
-	crc = crc32c(crc, buffer, sizeof(struct sctphdr) - sizeof(__u32));
+	crc = sctp_crc32c(crc, buffer, sizeof(struct sctphdr) - sizeof(__u32));
 
 	/* Skip checksum field of the header. */
-	crc = crc32c(crc, zero, sizeof(__u32));
+	crc = sctp_crc32c(crc, zero, sizeof(__u32));
 
 	/* Calculate the rest of the CRC. */
-	crc = crc32c(crc, &buffer[sizeof(struct sctphdr)],
+	crc = sctp_crc32c(crc, &buffer[sizeof(struct sctphdr)],
 			    length - sizeof(struct sctphdr));
 	return crc;
 }
 
-static inline __u32 sctp_update_cksum(__u8 *buffer, __u16 length, __u32 crc32)
+static inline __be32 sctp_update_cksum(__u8 *buffer, __u16 length, __be32 crc32)
 {
-	return crc32c(crc32, buffer, length);
+	return sctp_crc32c(crc32, buffer, length);
 }
 
-static inline __u32 sctp_end_cksum(__u32 crc32)
+static inline __be32 sctp_end_cksum(__be32 crc32)
 {
-	return ntohl(~crc32);
+	return ~crc32;
 }

net/ipv4/netfilter/nf_nat_proto_sctp.c +2 -2

index 82e4c0e..65e470b 100644
@@ -36,7 +36,7 @@ sctp_manip_pkt(struct sk_buff *skb,
 	sctp_sctphdr_t *hdr;
 	unsigned int hdroff = iphdroff + iph->ihl*4;
 	__be32 oldip, newip;
-	u32 crc32;
+	__be32 crc32;
 
 	if (!skb_make_writable(skb, hdroff + sizeof(*hdr)))
 		return false;

@@ -61,7 +61,7 @@ sctp_manip_pkt(struct sk_buff *skb,
 		crc32 = sctp_update_cksum((u8 *)skb->data, skb_headlen(skb),
 					  crc32);
 	crc32 = sctp_end_cksum(crc32);
-	hdr->checksum = htonl(crc32);
+	hdr->checksum = crc32;
 
 	return true;
 }

net/sctp/input.c +2 -2

index d354a23..463d529 100644
@@ -82,8 +82,8 @@ static inline int sctp_rcv_checksum(struct sk_buff *skb)
 {
 	struct sk_buff *list = skb_shinfo(skb)->frag_list;
 	struct sctphdr *sh = sctp_hdr(skb);
-	__u32 cmp = ntohl(sh->checksum);
-	__u32 val = sctp_start_cksum((__u8 *)sh, skb_headlen(skb));
+	__be32 cmp = sh->checksum;
+	__be32 val = sctp_start_cksum((__u8 *)sh, skb_headlen(skb));
 
 	for (; list; list = list->next)
 		val = sctp_update_cksum((__u8 *)list->data, skb_headlen(list),

net/sctp/output.c +2 -2

index abcd00d..e2158ad 100644
@@ -364,7 +364,7 @@ int sctp_packet_transmit(struct sctp_packet *packet)
 	struct sctp_transport *tp = packet->transport;
 	struct sctp_association *asoc = tp->asoc;
 	struct sctphdr *sh;
-	__u32 crc32 = 0;
+	__be32 crc32 = cpu_to_be32(0);
 	struct sk_buff *nskb;
 	struct sctp_chunk *chunk, *tmp;
 	struct sock *sk;

@@ -537,7 +537,7 @@ int sctp_packet_transmit(struct sctp_packet *packet)
 	/* 3) Put the resultant value into the checksum field in the
 	 *    common header, and leave the rest of the bits unchanged.
 	 */
-	sh->checksum = htonl(crc32);
+	sh->checksum = crc32;
 
 	/* IP layer ECN support
 	 * From RFC 2481

Signature · 1 line

1.5.6.1.281.g1087e

Vlad Yasevich

2008-07-09 20:59 UTC #1 lore raw

Quoted 102 lines · Harvey Harrison

Signed-off-by Harvey Harrison <[email protected]>

---
 include/net/sctp/checksum.h            |   23 ++++++++++++++---------
 net/ipv4/netfilter/nf_nat_proto_sctp.c |    4 ++--
 net/sctp/input.c                       |    4 ++--
 net/sctp/output.c                      |    4 ++--
 4 files changed, 20 insertions(+), 15 deletions(-)

include/net/sctp/checksum.h +14 -9

index ba75c67..b799fb2 100644
@@ -46,9 +46,14 @@
 #include <net/sctp/sctp.h>
 #include <linux/crc32c.h>
 
-static inline __u32 sctp_start_cksum(__u8 *buffer, __u16 length)
+static inline __be32 sctp_crc32c(__be32 crc, u8 *buffer, u16 length)
 {
-	__u32 crc = ~(__u32) 0;
+	return (__force __be32)crc32c((__force u32)crc, buffer, length);
+}
+
+static inline __be32 sctp_start_cksum(__u8 *buffer, __u16 length)
+{
+	__be32 crc = ~cpu_to_be32(0);
 	__u8  zero[sizeof(__u32)] = {0};
 
 	/* Optimize this routine to be SCTP specific, knowing how

@@ -56,23 +61,23 @@ static inline __u32 sctp_start_cksum(__u8 *buffer, __u16 length)
 	 */
 
 	/* Calculate CRC up to the checksum. */
-	crc = crc32c(crc, buffer, sizeof(struct sctphdr) - sizeof(__u32));
+	crc = sctp_crc32c(crc, buffer, sizeof(struct sctphdr) - sizeof(__u32));
 
 	/* Skip checksum field of the header. */
-	crc = crc32c(crc, zero, sizeof(__u32));
+	crc = sctp_crc32c(crc, zero, sizeof(__u32));
 
 	/* Calculate the rest of the CRC. */
-	crc = crc32c(crc, &buffer[sizeof(struct sctphdr)],
+	crc = sctp_crc32c(crc, &buffer[sizeof(struct sctphdr)],
 			    length - sizeof(struct sctphdr));
 	return crc;
 }
 
-static inline __u32 sctp_update_cksum(__u8 *buffer, __u16 length, __u32 crc32)
+static inline __be32 sctp_update_cksum(__u8 *buffer, __u16 length, __be32 crc32)
 {
-	return crc32c(crc32, buffer, length);
+	return sctp_crc32c(crc32, buffer, length);
 }
 
-static inline __u32 sctp_end_cksum(__u32 crc32)
+static inline __be32 sctp_end_cksum(__be32 crc32)
 {
-	return ntohl(~crc32);
+	return ~crc32;
 }

net/ipv4/netfilter/nf_nat_proto_sctp.c +2 -2

index 82e4c0e..65e470b 100644
@@ -36,7 +36,7 @@ sctp_manip_pkt(struct sk_buff *skb,
 	sctp_sctphdr_t *hdr;
 	unsigned int hdroff = iphdroff + iph->ihl*4;
 	__be32 oldip, newip;
-	u32 crc32;
+	__be32 crc32;
 
 	if (!skb_make_writable(skb, hdroff + sizeof(*hdr)))
 		return false;

@@ -61,7 +61,7 @@ sctp_manip_pkt(struct sk_buff *skb,
 		crc32 = sctp_update_cksum((u8 *)skb->data, skb_headlen(skb),
 					  crc32);
 	crc32 = sctp_end_cksum(crc32);
-	hdr->checksum = htonl(crc32);
+	hdr->checksum = crc32;
 
 	return true;
 }

net/sctp/input.c +2 -2

index d354a23..463d529 100644
@@ -82,8 +82,8 @@ static inline int sctp_rcv_checksum(struct sk_buff *skb)
 {
 	struct sk_buff *list = skb_shinfo(skb)->frag_list;
 	struct sctphdr *sh = sctp_hdr(skb);
-	__u32 cmp = ntohl(sh->checksum);
-	__u32 val = sctp_start_cksum((__u8 *)sh, skb_headlen(skb));
+	__be32 cmp = sh->checksum;
+	__be32 val = sctp_start_cksum((__u8 *)sh, skb_headlen(skb));
 
 	for (; list; list = list->next)
 		val = sctp_update_cksum((__u8 *)list->data, skb_headlen(list),

net/sctp/output.c +0 -0

index abcd00d..e2158ad 100644

net/sctp/output.c int sctp_packet_transmit(struct sctp_packet *packet) · line 367

Quoted 5 lines

net/sctp/output.c +1 -1

@@ -364,7 +364,7 @@ int sctp_packet_transmit(struct sctp_packet *packet)
 	struct sctp_transport *tp = packet->transport;
 	struct sctp_association *asoc = tp->asoc;
 	struct sctphdr *sh;
-	__u32 crc32 = 0;
+	__be32 crc32 = cpu_to_be32(0);

Does this actually do anything?

Otherwise, good clean-up. Thanks

-vlad

Quoted 12 lines

net/sctp/output.c +1 -1

 	struct sk_buff *nskb;
 	struct sctp_chunk *chunk, *tmp;
 	struct sock *sk;

@@ -537,7 +537,7 @@ int sctp_packet_transmit(struct sctp_packet *packet)
 	/* 3) Put the resultant value into the checksum field in the
 	 *    common header, and leave the rest of the bits unchanged.
 	 */
-	sh->checksum = htonl(crc32);
+	sh->checksum = crc32;
 
 	/* IP layer ECN support
 	 * From RFC 2481

Harvey Harrison

author

2008-07-10 03:28 UTC #2 lore raw

Quoted 8 lines · Vlad Yasevich

net/sctp/output.c int sctp_packet_transmit(struct sctp_packet *packet) · line 367
Quoted 2 lines · Harvey Harrison
net/sctp/output.c +1 -1
-	__u32 crc32 = 0;
+	__be32 crc32 = cpu_to_be32(0);
Does this actually do anything?
Otherwise, good clean-up. Thanks

Keeps sparse happy.

Harvey

Vlad Yasevich

2008-07-10 12:43 UTC #3 lore raw

Quoted 10 lines · Harvey Harrison

Quoted 7 lines · Vlad Yasevich
net/sctp/output.c int sctp_packet_transmit(struct sctp_packet *packet) · line 367
Quoted 2 lines · Harvey Harrison
net/sctp/output.c +1 -1
-	__u32 crc32 = 0;
+	__be32 crc32 = cpu_to_be32(0);
Does this actually do anything?
Otherwise, good clean-up. Thanks
Keeps sparse happy.

Wouldn't just a (__force __be32)0 do the same thing and avoid useless byte swapping on little endian?

-vald

Quoted 3 lines

Harvey

Harvey Harrison

author

2008-07-11 18:07 UTC #4 lore raw

Quoted 7 lines · Vlad Yasevich

Quoted 6 lines · Harvey Harrison
Quoted 5 lines · Vlad Yasevich
net/sctp/output.c int sctp_packet_transmit(struct sctp_packet *packet) · line 367
Quoted 2 lines · Harvey Harrison
net/sctp/output.c +1 -1
-	__u32 crc32 = 0;
+	__be32 crc32 = cpu_to_be32(0);
Does this actually do anything?

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
Otherwise, good clean-up. Thanks
Keeps sparse happy.
Wouldn't just a (__force __be32)0 do the same thing and avoid useless byte swapping on little endian?

I'm trying to get a patch accepted that makes cpu_to_be32 do compile-time folding if possible, so in the future this won't produce any additional code. For now, just use __constant_cpu_to_be32(0)

Harvey

Harvey Harrison

author

2008-07-17 19:21 UTC #5 lore raw

In reply to Vlad Yasevich

Quoted 7 lines · Vlad Yasevich

Quoted 6 lines · Harvey Harrison
Quoted 5 lines · Vlad Yasevich
net/sctp/output.c int sctp_packet_transmit(struct sctp_packet *packet) · line 367
Quoted 2 lines · Harvey Harrison
net/sctp/output.c +1 -1
-	__u32 crc32 = 0;
+	__be32 crc32 = cpu_to_be32(0);
Does this actually do anything?

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
Otherwise, good clean-up. Thanks
Keeps sparse happy.
Wouldn't just a (__force __be32)0 do the same thing and avoid useless byte swapping on little endian?

Vlad, do you want me to revise the patch, or were you taking it as is?

Harvey

Vlad Yasevich

2008-07-17 19:26 UTC #6 lore raw

Quoted 10 lines · Harvey Harrison

Quoted 9 lines · Vlad Yasevich
Quoted 8 lines · Harvey Harrison
Quoted 7 lines · Vlad Yasevich
net/sctp/output.c int sctp_packet_transmit(struct sctp_packet *packet) · line 367
Quoted 2 lines · Harvey Harrison
net/sctp/output.c +1 -1
-	__u32 crc32 = 0;
+	__be32 crc32 = cpu_to_be32(0);
Does this actually do anything?
Otherwise, good clean-up. Thanks

Quoted 9 lines

Quoted 4 lines
Quoted 1 line
Keeps sparse happy.
Wouldn't just a (__force __be32)0 do the same thing and avoid useless byte swapping on little endian?
Vlad, do you want me to revise the patch, or were you taking it as is?
Harvey

Could you fix it please.

Thanks
-vlad

Harvey Harrison

author

2008-07-17 19:31 UTC #7 lore raw

PATCH v2 1/1 sctp: remove unnecessary byteshifting, calculate directly in big-endian

Signed-off-by Harvey Harrison <[email protected]>

---
Vlad, I went with __constant_cpu_to_be32 which does the compile-time folding, rather than the __force cast.

 include/net/sctp/checksum.h            |   23 ++++++++++++++---------
 net/ipv4/netfilter/nf_nat_proto_sctp.c |    4 ++--
 net/sctp/input.c                       |    4 ++--
 net/sctp/output.c                      |    4 ++--
 4 files changed, 20 insertions(+), 15 deletions(-)

include/net/sctp/checksum.h +14 -9

index ba75c67..b799fb2 100644
@@ -46,9 +46,14 @@
 #include <net/sctp/sctp.h>
 #include <linux/crc32c.h>
 
-static inline __u32 sctp_start_cksum(__u8 *buffer, __u16 length)
+static inline __be32 sctp_crc32c(__be32 crc, u8 *buffer, u16 length)
 {
-	__u32 crc = ~(__u32) 0;
+	return (__force __be32)crc32c((__force u32)crc, buffer, length);
+}
+
+static inline __be32 sctp_start_cksum(__u8 *buffer, __u16 length)
+{
+	__be32 crc = ~cpu_to_be32(0);
 	__u8  zero[sizeof(__u32)] = {0};
 
 	/* Optimize this routine to be SCTP specific, knowing how

@@ -56,23 +61,23 @@ static inline __u32 sctp_start_cksum(__u8 *buffer, __u16 length)
 	 */
 
 	/* Calculate CRC up to the checksum. */
-	crc = crc32c(crc, buffer, sizeof(struct sctphdr) - sizeof(__u32));
+	crc = sctp_crc32c(crc, buffer, sizeof(struct sctphdr) - sizeof(__u32));
 
 	/* Skip checksum field of the header. */
-	crc = crc32c(crc, zero, sizeof(__u32));
+	crc = sctp_crc32c(crc, zero, sizeof(__u32));
 
 	/* Calculate the rest of the CRC. */
-	crc = crc32c(crc, &buffer[sizeof(struct sctphdr)],
+	crc = sctp_crc32c(crc, &buffer[sizeof(struct sctphdr)],
 			    length - sizeof(struct sctphdr));
 	return crc;
 }
 
-static inline __u32 sctp_update_cksum(__u8 *buffer, __u16 length, __u32 crc32)
+static inline __be32 sctp_update_cksum(__u8 *buffer, __u16 length, __be32 crc32)
 {
-	return crc32c(crc32, buffer, length);
+	return sctp_crc32c(crc32, buffer, length);
 }
 
-static inline __u32 sctp_end_cksum(__u32 crc32)
+static inline __be32 sctp_end_cksum(__be32 crc32)
 {
-	return ntohl(~crc32);
+	return ~crc32;
 }

net/ipv4/netfilter/nf_nat_proto_sctp.c +2 -2

index 82e4c0e..65e470b 100644
@@ -36,7 +36,7 @@ sctp_manip_pkt(struct sk_buff *skb,
 	sctp_sctphdr_t *hdr;
 	unsigned int hdroff = iphdroff + iph->ihl*4;
 	__be32 oldip, newip;
-	u32 crc32;
+	__be32 crc32;
 
 	if (!skb_make_writable(skb, hdroff + sizeof(*hdr)))
 		return false;

@@ -61,7 +61,7 @@ sctp_manip_pkt(struct sk_buff *skb,
 		crc32 = sctp_update_cksum((u8 *)skb->data, skb_headlen(skb),
 					  crc32);
 	crc32 = sctp_end_cksum(crc32);
-	hdr->checksum = htonl(crc32);
+	hdr->checksum = crc32;
 
 	return true;
 }

net/sctp/input.c +2 -2

index d354a23..463d529 100644
@@ -82,8 +82,8 @@ static inline int sctp_rcv_checksum(struct sk_buff *skb)
 {
 	struct sk_buff *list = skb_shinfo(skb)->frag_list;
 	struct sctphdr *sh = sctp_hdr(skb);
-	__u32 cmp = ntohl(sh->checksum);
-	__u32 val = sctp_start_cksum((__u8 *)sh, skb_headlen(skb));
+	__be32 cmp = sh->checksum;
+	__be32 val = sctp_start_cksum((__u8 *)sh, skb_headlen(skb));
 
 	for (; list; list = list->next)
 		val = sctp_update_cksum((__u8 *)list->data, skb_headlen(list),

net/sctp/output.c +2 -2

index abcd00d..e2158ad 100644
@@ -364,7 +364,7 @@ int sctp_packet_transmit(struct sctp_packet *packet)
 	struct sctp_transport *tp = packet->transport;
 	struct sctp_association *asoc = tp->asoc;
 	struct sctphdr *sh;
-	__u32 crc32 = 0;
+	__be32 crc32 = __constant_cpu_to_be32(0);
 	struct sk_buff *nskb;
 	struct sctp_chunk *chunk, *tmp;
 	struct sock *sk;

@@ -537,7 +537,7 @@ int sctp_packet_transmit(struct sctp_packet *packet)
 	/* 3) Put the resultant value into the checksum field in the
 	 *    common header, and leave the rest of the bits unchanged.
 	 */
-	sh->checksum = htonl(crc32);
+	sh->checksum = crc32;
 
 	/* IP layer ECN support
 	 * From RFC 2481

Signature · 1 line

1.5.6.1.281.g1087e