ceph: minor cleanups

PATCH In review Alex Elder 2 participants 6 messages Feb 29, 2012 – Mar 2, 2012 Ceph common code (libceph)

Share & tools

Alex Elder

author

2012-02-29 03:09 UTC #0 lore raw

cover ceph: minor cleanups

These are mostly small cleanups. Only the second one has any significant changes in it--using a masked atomic rather than a spinlocked mod operation for determining an index to use on a fixed-size array.

-Alex

Alex Elder

author

2012-02-29 03:12 UTC #1 lore raw

PATCH 1/4 ceph: make use of "else" where appropriate

Rearrange ceph_tcp_connect() a bit, making use of "else" rather than re-testing a value with consecutive "if" statements. Don't record a connection's socket pointer unless the connect operation is successful.

Signed-off-by Alex Elder <[email protected]>

---
 net/ceph/messenger.c |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

net/ceph/messenger.c +4 -7

index 068cb7e..c9a413a 100644
@@ -247,7 +247,6 @@ static struct socket *ceph_tcp_connect(struct ceph_connection *con)
 			       IPPROTO_TCP, &sock);
 	if (ret)
 		return ERR_PTR(ret);
-	con->sock = sock;
 	sock->sk->sk_allocation = GFP_NOFS;
 
 #ifdef CONFIG_LOCKDEP

@@ -264,18 +263,16 @@ static struct socket *ceph_tcp_connect(struct ceph_connection *con)
 		dout("connect %s EINPROGRESS sk_state = %u\n",
 		     ceph_pr_addr(&con->peer_addr.in_addr),
 		     sock->sk->sk_state);
-		ret = 0;
-	}
-	if (ret < 0) {
+	} else if (ret < 0) {
 		pr_err("connect %s error %d\n",
 		       ceph_pr_addr(&con->peer_addr.in_addr), ret);
 		sock_release(sock);
-		con->sock = NULL;
 		con->error_msg = "connect error";
-	}
 
-	if (ret < 0)
 		return ERR_PTR(ret);
+	}
+	con->sock = sock;
+
 	return sock;
 }

Signature · 1 line

1.7.5.4

Alex Elder

author

2012-02-29 03:12 UTC #2 lore raw

PATCH 2/4 ceph: kill addr_str_lock spinlock; use atomic instead

In reply to Alex Elder

A spinlock is used to protect a value used for selecting an array index for a string used for formatting a socket address for human consumption. The index is reset to 0 if it ever reaches the maximum index value.

Instead, use an ever-increasing atomic variable as a sequence number, and compute the array index by masking off all but the sequence number's lowest bits. Make the number of entries in the array a power of two to allow the use of such a mask (to avoid jumps in the index value when the sequence number wraps).

The length of these strings is somewhat arbitrarily set at 60 bytes. The worst-case length of a string produced is 54 bytes, for an IPv6 address that can't be shortened, e.g.:

    [1234:5678:9abc:def0:1111:2222:123.234.210.100]:32767

Change it so we arbitrarily use 64 bytes instead; if nothing else it will make the array of these line up better in hex dumps.

Rename a few things to reinforce the distinction between the number of strings in the array and the length of individual strings.

Signed-off-by Alex Elder <[email protected]>

---
 net/ceph/messenger.c |   21 ++++++++++-----------
 1 files changed, 10 insertions(+), 11 deletions(-)

net/ceph/messenger.c +10 ~~-11~~

index c9a413a..8edfd1a 100644
@@ -44,13 +44,16 @@ static void con_work(struct work_struct *);
 static void ceph_fault(struct ceph_connection *con);
 
 /*
- * nicely render a sockaddr as a string.
+ * Nicely render a sockaddr as a string.  An array of formatted
+ * strings is used, to approximate reentrancy.
  */
-#define MAX_ADDR_STR 20
-#define MAX_ADDR_STR_LEN 60
-static char addr_str[MAX_ADDR_STR][MAX_ADDR_STR_LEN];
-static DEFINE_SPINLOCK(addr_str_lock);
-static int last_addr_str;
+#define ADDR_STR_COUNT_LOG	5	/* log2(# address strings in array) */
+#define ADDR_STR_COUNT		(1 << ADDR_STR_COUNT_LOG)
+#define ADDR_STR_COUNT_MASK	(ADDR_STR_COUNT - 1)
+#define MAX_ADDR_STR_LEN	64	/* 54 is enough */
+
+static char addr_str[ADDR_STR_COUNT][MAX_ADDR_STR_LEN];
+static atomic_t addr_str_seq = ATOMIC_INIT(0);
 
 static struct page *zero_page;		/* used in certain error cases */
 static void *zero_page_address;		/* kernel virtual addr of zero_page */

@@ -62,11 +65,7 @@ const char *ceph_pr_addr(const struct sockaddr_storage *ss)
 	struct sockaddr_in *in4 = (void *)ss;
 	struct sockaddr_in6 *in6 = (void *)ss;
 
-	spin_lock(&addr_str_lock);
-	i = last_addr_str++;
-	if (last_addr_str == MAX_ADDR_STR)
-		last_addr_str = 0;
-	spin_unlock(&addr_str_lock);
+	i = atomic_inc_return(&addr_str_seq) & ADDR_STR_COUNT_MASK;
 	s = addr_str[i];
 
 	switch (ss->ss_family) {

Signature · 1 line

1.7.5.4

Alex Elder

author

2012-02-29 03:12 UTC #3 lore raw

PATCH 3/4 ceph: eliminate some needless casts

In reply to Alex Elder

This eliminates type casts in some places where they are not required.

Signed-off-by Alex Elder <[email protected]>

---
 net/ceph/messenger.c |   21 ++++++++++-----------
 1 files changed, 10 insertions(+), 11 deletions(-)

net/ceph/messenger.c +10 ~~-11~~

index 8edfd1a..e95756a 100644
@@ -70,13 +70,13 @@ const char *ceph_pr_addr(const struct sockaddr_storage *ss)
 
 	switch (ss->ss_family) {
 	case AF_INET:
-		snprintf(s, MAX_ADDR_STR_LEN, "%pI4:%u", &in4->sin_addr,
-			 (unsigned int)ntohs(in4->sin_port));
+		snprintf(s, MAX_ADDR_STR_LEN, "%pI4:%hu", &in4->sin_addr,
+			 ntohs(in4->sin_port));
 		break;
 
 	case AF_INET6:
-		snprintf(s, MAX_ADDR_STR_LEN, "[%pI6c]:%u", &in6->sin6_addr,
-			 (unsigned int)ntohs(in6->sin6_port));
+		snprintf(s, MAX_ADDR_STR_LEN, "[%pI6c]:%hu", &in6->sin6_addr,
+			 ntohs(in6->sin6_port));
 		break;
 
 	default:

@@ -155,8 +155,8 @@ EXPORT_SYMBOL(ceph_msgr_flush);
 /* data available on socket, or listen socket received a connect */
 static void ceph_data_ready(struct sock *sk, int count_unused)
 {
-	struct ceph_connection *con =
-		(struct ceph_connection *)sk->sk_user_data;
+	struct ceph_connection *con = sk->sk_user_data;
+
 	if (sk->sk_state != TCP_CLOSE_WAIT) {
 		dout("ceph_data_ready on %p state = %lu, queueing work\n",
 		     con, con->state);

@@ -185,8 +185,7 @@ static void ceph_write_space(struct sock *sk)
 /* socket's state has changed */
 static void ceph_state_change(struct sock *sk)
 {
-	struct ceph_connection *con =
-		(struct ceph_connection *)sk->sk_user_data;
+	struct ceph_connection *con = sk->sk_user_data;
 
 	dout("ceph_state_change %p state = %lu sk_state = %u\n",
 	     con, con->state, sk->sk_state);

@@ -221,7 +220,7 @@ static void set_sock_callbacks(struct socket *sock,
 			       struct ceph_connection *con)
 {
 	struct sock *sk = sock->sk;
-	sk->sk_user_data = (void *)con;
+	sk->sk_user_data = con;
 	sk->sk_data_ready = ceph_data_ready;
 	sk->sk_write_space = ceph_write_space;
 	sk->sk_state_change = ceph_state_change;

@@ -548,7 +547,7 @@ static void prepare_write_message(struct ceph_connection *con)
 
 	/* fill in crc (except data pages), footer */
 	con->out_msg->hdr.crc =
-		cpu_to_le32(crc32c(0, (void *)&m->hdr,
+		cpu_to_le32(crc32c(0, &m->hdr,
 				      sizeof(m->hdr) - sizeof(m->hdr.crc)));
 	con->out_msg->footer.flags = CEPH_MSG_FOOTER_COMPLETE;
 	con->out_msg->footer.front_crc =

@@ -1643,7 +1642,7 @@ static int read_partial_message(struct ceph_connection *con)
 			return ret;
 		con->in_base_pos += ret;
 		if (con->in_base_pos == sizeof(con->in_hdr)) {
-			u32 crc = crc32c(0, (void *)&con->in_hdr,
+			u32 crc = crc32c(0, &con->in_hdr,
 				 sizeof(con->in_hdr) - sizeof(con->in_hdr.crc));
 			if (crc != le32_to_cpu(con->in_hdr.crc)) {
 				pr_err("read_partial_message bad hdr "

Signature · 1 line

1.7.5.4

Alex Elder

author

2012-02-29 03:12 UTC #4 lore raw

PATCH 4/4 ceph: eliminate some abusive casts

In reply to Alex Elder

This fixes some spots where a type cast to (void *) was used as as a universal type hiding mechanism. Instead, properly cast the type to the intended target type.

Signed-off-by Alex Elder <[email protected]>

---
 net/ceph/messenger.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

net/ceph/messenger.c +2 -2

index e95756a..c410c55 100644
@@ -62,8 +62,8 @@ const char *ceph_pr_addr(const struct sockaddr_storage *ss)
 {
 	int i;
 	char *s;
-	struct sockaddr_in *in4 = (void *)ss;
-	struct sockaddr_in6 *in6 = (void *)ss;
+	struct sockaddr_in *in4 = (struct sockaddr_in *) ss;
+	struct sockaddr_in6 *in6 = (struct sockaddr_in6 *) ss;
 
 	i = atomic_inc_return(&addr_str_seq) & ADDR_STR_COUNT_MASK;
 	s = addr_str[i];

@@ -1108,8 +1108,8 @@ static void addr_set_port(struct sockaddr_storage *ss, int p)
 static int ceph_pton(const char *str, size_t len, struct

sockaddr_storage *ss,
char delim, const char **ipend)
{
- struct sockaddr_in *in4 = (void *)ss;
- struct sockaddr_in6 *in6 = (void *)ss;
+ struct sockaddr_in *in4 = (struct sockaddr_in *) ss;
+ struct sockaddr_in6 *in6 = (struct sockaddr_in6 *) ss;

memset(ss, 0, sizeof(*ss));

Signature · 1 line

1.7.5.4

Sage Weil

Reviewed-by on the v1 series

2012-03-02 19:25 UTC #5 lore raw

In reply to Alex Elder

This set looks good,

Reviewed-by Sage Weil <[email protected]>

Quoted 12 lines · Alex Elder

These are mostly small cleanups. Only the second one has any significant changes in it--using a masked atomic rather than a spinlocked mod operation for determining an index to use on a fixed-size array.
-Alex
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html