[rfc] larger batches for crc32c

* [rfc] larger batches for crc32c
@ 2016-10-27 16:17 Nicholas Piggin
  2016-10-27 18:29 ` Darrick J. Wong
                   ` (2 more replies)
  0 siblings, 3 replies; 20+ messages in thread
From: Nicholas Piggin @ 2016-10-27 16:17 UTC (permalink / raw)
  To: linux-xfs; +Cc: Christoph Hellwig, Dave Chinner

Hi guys,

We're seeing crc32c_le show up in xfs log checksumming on a MySQL benchmark
on powerpc. I could reproduce similar overheads with dbench as well.

1.11%  mysqld           [kernel.vmlinux]            [k] __crc32c_le
        |
        ---__crc32c_le
           |          
            --1.11%--chksum_update
                      |          
                       --1.11%--crypto_shash_update
                                 crc32c
                                 xlog_cksum
                                 xlog_sync
                                 _xfs_log_force_lsn
                                 xfs_file_fsync
                                 vfs_fsync_range
                                 do_fsync
                                 sys_fsync
                                 system_call
                                 0x17738
                                 0x17704
                                 os_file_flush_func
                                 fil_flush

As a rule, it helps the crc implementation if it can operate on as large a
chunk as possible (alignment, startup overhead, etc). So I did a quick hack
at getting XFS checksumming to feed crc32c() with larger chunks, by setting
the existing crc to 0 before running over the entire buffer. Together with
some small work on the powerpc crc implementation, crc drops below 0.1%.

I don't know if something like this would be acceptable? It's not pretty,
but I didn't see an easier way.

diff --git a/fs/xfs/libxfs/xfs_cksum.h b/fs/xfs/libxfs/xfs_cksum.h
index fad1676..88f4431 100644
--- a/fs/xfs/libxfs/xfs_cksum.h
+++ b/fs/xfs/libxfs/xfs_cksum.h
@@ -9,20 +9,9 @@
  * cksum_offset parameter.
  */
 static inline __uint32_t
-xfs_start_cksum(char *buffer, size_t length, unsigned long cksum_offset)
+xfs_start_cksum(void *buffer, size_t length)
 {
-	__uint32_t zero = 0;
-	__uint32_t crc;
-
-	/* Calculate CRC up to the checksum. */
-	crc = crc32c(XFS_CRC_SEED, buffer, cksum_offset);
-
-	/* Skip checksum field */
-	crc = crc32c(crc, &zero, sizeof(__u32));
-
-	/* Calculate the rest of the CRC. */
-	return crc32c(crc, &buffer[cksum_offset + sizeof(__be32)],
-		      length - (cksum_offset + sizeof(__be32)));
+	return crc32c(XFS_CRC_SEED, buffer, length);
 }
 
 /*
@@ -42,22 +31,29 @@ xfs_end_cksum(__uint32_t crc)
  * Helper to generate the checksum for a buffer.
  */
 static inline void
-xfs_update_cksum(char *buffer, size_t length, unsigned long cksum_offset)
+xfs_update_cksum(void *buffer, size_t length, unsigned long cksum_offset)
 {
-	__uint32_t crc = xfs_start_cksum(buffer, length, cksum_offset);
+	__le32 *crcp = buffer + cksum_offset;
 
-	*(__le32 *)(buffer + cksum_offset) = xfs_end_cksum(crc);
+	*crcp = 0; /* set to 0 before calculating checksum */
+	*crcp = xfs_end_cksum(xfs_start_cksum(buffer, length));
 }
 
 /*
  * Helper to verify the checksum for a buffer.
  */
 static inline int
-xfs_verify_cksum(char *buffer, size_t length, unsigned long cksum_offset)
+xfs_verify_cksum(void *buffer, size_t length, unsigned long cksum_offset)
 {
-	__uint32_t crc = xfs_start_cksum(buffer, length, cksum_offset);
+	__le32 *crcp = buffer + cksum_offset;
+	__le32 crc = *crcp;
+	__le32 new_crc;
+
+	*crcp = 0;
+	new_crc = xfs_end_cksum(xfs_start_cksum(buffer, length));
+	*crcp = crc; /* restore original CRC in place */
 
-	return *(__le32 *)(buffer + cksum_offset) == xfs_end_cksum(crc);
+	return new_crc == crc;
 }
 
 #endif /* _XFS_CKSUM_H */
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index 8de9a3a..efd8daa 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -419,15 +419,11 @@ xfs_dinode_calc_crc(
 	struct xfs_mount	*mp,
 	struct xfs_dinode	*dip)
 {
-	__uint32_t		crc;
-
 	if (dip->di_version < 3)
 		return;
 
 	ASSERT(xfs_sb_version_hascrc(&mp->m_sb));
-	crc = xfs_start_cksum((char *)dip, mp->m_sb.sb_inodesize,
-			      XFS_DINODE_CRC_OFF);
-	dip->di_crc = xfs_end_cksum(crc);
+	xfs_update_cksum(dip, mp->m_sb.sb_inodesize, XFS_DINODE_CRC_OFF);
 }
 
 /*
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 3b74fa0..4e242f0 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1658,7 +1658,7 @@ xlog_pack_data(
  * This is a little more complicated than it should be because the various
  * headers and the actual data are non-contiguous.
  */
-__le32
+void
 xlog_cksum(
 	struct xlog		*log,
 	struct xlog_rec_header	*rhead,
@@ -1667,10 +1667,9 @@ xlog_cksum(
 {
 	__uint32_t		crc;
 
-	/* first generate the crc for the record header ... */
-	crc = xfs_start_cksum((char *)rhead,
-			      sizeof(struct xlog_rec_header),
-			      offsetof(struct xlog_rec_header, h_crc));
+	/* first generate the crc for the record header with 0 crc... */
+	rhead->h_crc = 0;
+	crc = xfs_start_cksum(rhead, sizeof(struct xlog_rec_header));
 
 	/* ... then for additional cycle data for v2 logs ... */
 	if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) {
@@ -1691,7 +1690,7 @@ xlog_cksum(
 	/* ... and finally for the payload */
 	crc = crc32c(crc, dp, size);
 
-	return xfs_end_cksum(crc);
+	rhead->h_crc = xfs_end_cksum(crc);
 }
 
 /*
@@ -1840,8 +1839,7 @@ xlog_sync(
 	}
 
 	/* calculcate the checksum */
-	iclog->ic_header.h_crc = xlog_cksum(log, &iclog->ic_header,
-					    iclog->ic_datap, size);
+	xlog_cksum(log, &iclog->ic_header, iclog->ic_datap, size);
 #ifdef DEBUG
 	/*
 	 * Intentionally corrupt the log record CRC based on the error injection
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index 2b6eec5..18ba385 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -432,7 +432,7 @@ xlog_recover_finish(
 extern int
 xlog_recover_cancel(struct xlog *);
 
-extern __le32	 xlog_cksum(struct xlog *log, struct xlog_rec_header *rhead,
+extern void	 xlog_cksum(struct xlog *log, struct xlog_rec_header *rhead,
 			    char *dp, int size);
 
 extern kmem_zone_t *xfs_log_ticket_zone;
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 9b3d7c7..3f62d9a 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -5114,8 +5114,13 @@ xlog_recover_process(
 {
 	int			error;
 	__le32			crc;
+	__le32			old_crc;
 
-	crc = xlog_cksum(log, rhead, dp, be32_to_cpu(rhead->h_len));
+	old_crc = rhead->h_crc;
+	rhead->h_crc = 0;
+	xlog_cksum(log, rhead, dp, be32_to_cpu(rhead->h_len));
+	crc = rhead->h_crc;
+	rhead->h_crc = old_crc;
 
 	/*
 	 * Nothing else to do if this is a CRC verification pass. Just return
-- 
2.9.3


^ permalink raw reply related	[flat|nested] 20+ messages in thread