This moves the list_empty() checks from the two callers (v1 and v2)
into the base messenger.c library.  Now the v1/v2 specializations do
not need to know about con->out_queue; that implementation detail is
now hidden behind the ceph_con_get_out_msg() function.

Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
---
 net/ceph/messenger.c    |  4 +++-
 net/ceph/messenger_v1.c | 15 ++++++++++-----
 net/ceph/messenger_v2.c |  4 ++--
 3 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 424fb2769b71..8886c38a55d2 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -2113,7 +2113,9 @@ struct ceph_msg *ceph_con_get_out_msg(struct ceph_connection *con)
 {
 	struct ceph_msg *msg;
 
-	BUG_ON(list_empty(&con->out_queue));
+	if (list_empty(&con->out_queue))
+		return NULL;
+
 	msg = list_first_entry(&con->out_queue, struct ceph_msg, list_head);
 	WARN_ON(msg->con != con);
 
diff --git a/net/ceph/messenger_v1.c b/net/ceph/messenger_v1.c
index 516f2eeb122a..5eb6cfdbc494 100644
--- a/net/ceph/messenger_v1.c
+++ b/net/ceph/messenger_v1.c
@@ -189,12 +189,18 @@ static void prepare_write_message_footer(struct ceph_connection *con, struct cep
 
 /*
  * Prepare headers for the next outgoing message.
+ *
+ * @return false if there are no outgoing messages
  */
-static void prepare_write_message(struct ceph_connection *con)
+static bool prepare_write_message(struct ceph_connection *con)
 {
 	struct ceph_msg *m;
 	u32 crc;
 
+	m = ceph_con_get_out_msg(con);
+	if (m == NULL)
+		return false;
+
 	con_out_kvec_reset(con);
 	con->v1.out_msg_done = false;
 
@@ -208,8 +214,6 @@ static void prepare_write_message(struct ceph_connection *con)
 			&con->v1.out_temp_ack);
 	}
 
-	m = ceph_con_get_out_msg(con);
-
 	dout("prepare_write_message %p seq %lld type %d len %d+%d+%zd\n",
 	     m, con->out_seq, le16_to_cpu(m->hdr.type),
 	     le32_to_cpu(m->hdr.front_len), le32_to_cpu(m->hdr.middle_len),
@@ -256,6 +260,8 @@ static void prepare_write_message(struct ceph_connection *con)
 	}
 
 	ceph_con_flag_set(con, CEPH_CON_F_WRITE_PENDING);
+
+	return true;
 }
 
 /*
@@ -1543,8 +1549,7 @@ int ceph_con_v1_try_write(struct ceph_connection *con)
 			goto more;
 		}
 		/* is anything else pending? */
-		if (!list_empty(&con->out_queue)) {
-			prepare_write_message(con);
+		if (prepare_write_message(con)) {
 			goto more;
 		}
 		if (con->in_seq > con->in_seq_acked) {
diff --git a/net/ceph/messenger_v2.c b/net/ceph/messenger_v2.c
index 90109fa0fe60..e0b5f2e2582d 100644
--- a/net/ceph/messenger_v2.c
+++ b/net/ceph/messenger_v2.c
@@ -3292,6 +3292,7 @@ static void finish_message(struct ceph_connection *con)
 
 static int populate_out_iter(struct ceph_connection *con)
 {
+	struct ceph_msg *msg;
 	int ret;
 
 	dout("%s con %p state %d out_state %d\n", __func__, con, con->state,
@@ -3337,8 +3338,7 @@ static int populate_out_iter(struct ceph_connection *con)
 			pr_err("prepare_keepalive2 failed: %d\n", ret);
 			return ret;
 		}
-	} else if (!list_empty(&con->out_queue)) {
-		struct ceph_msg *msg = ceph_con_get_out_msg(con);
+	} else if ((msg = ceph_con_get_out_msg(con)) != NULL) {
 		ret = prepare_message(con, msg);
 		if (ret) {
 			pr_err("prepare_message failed: %d\n", ret);
-- 
2.47.2

On Wed, Aug 6, 2025 at 11:49 AM Max Kellermann <max.kellermann@ionos.com> wrote:
>
> This moves the list_empty() checks from the two callers (v1 and v2)
> into the base messenger.c library.  Now the v1/v2 specializations do
> not need to know about con->out_queue; that implementation detail is
> now hidden behind the ceph_con_get_out_msg() function.
>
> Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
> ---
>  net/ceph/messenger.c    |  4 +++-
>  net/ceph/messenger_v1.c | 15 ++++++++++-----
>  net/ceph/messenger_v2.c |  4 ++--
>  3 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
> index 424fb2769b71..8886c38a55d2 100644
> --- a/net/ceph/messenger.c
> +++ b/net/ceph/messenger.c
> @@ -2113,7 +2113,9 @@ struct ceph_msg *ceph_con_get_out_msg(struct ceph_connection *con)
>  {
>         struct ceph_msg *msg;
>
> -       BUG_ON(list_empty(&con->out_queue));
> +       if (list_empty(&con->out_queue))
> +               return NULL;
> +
>         msg = list_first_entry(&con->out_queue, struct ceph_msg, list_head);
>         WARN_ON(msg->con != con);
>
> diff --git a/net/ceph/messenger_v1.c b/net/ceph/messenger_v1.c
> index 516f2eeb122a..5eb6cfdbc494 100644
> --- a/net/ceph/messenger_v1.c
> +++ b/net/ceph/messenger_v1.c
> @@ -189,12 +189,18 @@ static void prepare_write_message_footer(struct ceph_connection *con, struct cep
>
>  /*
>   * Prepare headers for the next outgoing message.
> + *
> + * @return false if there are no outgoing messages
>   */
> -static void prepare_write_message(struct ceph_connection *con)
> +static bool prepare_write_message(struct ceph_connection *con)
>  {
>         struct ceph_msg *m;
>         u32 crc;
>
> +       m = ceph_con_get_out_msg(con);
> +       if (m == NULL)
> +               return false;
> +
>         con_out_kvec_reset(con);
>         con->v1.out_msg_done = false;
>
> @@ -208,8 +214,6 @@ static void prepare_write_message(struct ceph_connection *con)
>                         &con->v1.out_temp_ack);
>         }
>
> -       m = ceph_con_get_out_msg(con);
> -
>         dout("prepare_write_message %p seq %lld type %d len %d+%d+%zd\n",
>              m, con->out_seq, le16_to_cpu(m->hdr.type),
>              le32_to_cpu(m->hdr.front_len), le32_to_cpu(m->hdr.middle_len),
> @@ -256,6 +260,8 @@ static void prepare_write_message(struct ceph_connection *con)
>         }
>
>         ceph_con_flag_set(con, CEPH_CON_F_WRITE_PENDING);
> +
> +       return true;
>  }
>
>  /*
> @@ -1543,8 +1549,7 @@ int ceph_con_v1_try_write(struct ceph_connection *con)
>                         goto more;
>                 }
>                 /* is anything else pending? */
> -               if (!list_empty(&con->out_queue)) {
> -                       prepare_write_message(con);
> +               if (prepare_write_message(con)) {

Hi Max,

I made a change to net/ceph/messenger_v1.c hunks of this patch to
follow what is done for msgr2 where ceph_con_get_out_msg() is called
outside of the prepare helper and the new message is passed in.
prepare_write_message() doesn't need to return a bool anymore.

Let me know if you see something wrong there:

https://github.com/ceph/ceph-client/commit/6140f1d43ba9425dc55b12bdfd8877b0c5118d9a

Thanks,

                Ilya

On Thu, Oct 9, 2025 at 1:18 PM Ilya Dryomov <idryomov@gmail.com> wrote:
> I made a change to net/ceph/messenger_v1.c hunks of this patch to
> follow what is done for msgr2 where ceph_con_get_out_msg() is called
> outside of the prepare helper and the new message is passed in.
> prepare_write_message() doesn't need to return a bool anymore.

But ... why?
Your change is not bad, but I don't believe it belongs in this patch,
because it is out of this patch's scope. It would have been a good
follow-up patch.

There are lots of unnecessary (and sometimes confusing) differences
between the v1 and v2 messengers, but unifying these is out of the
scope of my patch. All my patch does is remove visibility of a
messenger.c implementation detail from the v1/v2 specializations.

(My end goal was to have unified multi-message send in one function
call to reduce overhead for sending bulk messages, but I did not yet
follow up on this idea yet. The Ceph kernel messenger, just like the
user-space messenger, leave a lot of room for optimizations -
unfortunately, my user-space optimizations that I submitted last year
were not merged by the Ceph project.)

Max

On Thu, Oct 9, 2025 at 1:47 PM Max Kellermann <max.kellermann@ionos.com> wrote:
>
> On Thu, Oct 9, 2025 at 1:18 PM Ilya Dryomov <idryomov@gmail.com> wrote:
> > I made a change to net/ceph/messenger_v1.c hunks of this patch to
> > follow what is done for msgr2 where ceph_con_get_out_msg() is called
> > outside of the prepare helper and the new message is passed in.
> > prepare_write_message() doesn't need to return a bool anymore.
>
> But ... why?
> Your change is not bad, but I don't believe it belongs in this patch,
> because it is out of this patch's scope. It would have been a good
> follow-up patch.

Changing a function to return a bool only to immediately undo that
change in a follow-up patch (both touching the same 10-15 lines of
code) seemed like pointless churn to me.

>
> There are lots of unnecessary (and sometimes confusing) differences
> between the v1 and v2 messengers, but unifying these is out of the
> scope of my patch. All my patch does is remove visibility of a
> messenger.c implementation detail from the v1/v2 specializations.

ceph_con_get_out_msg() is a common helper and given that this series
changed its signature and how it's supposed to be used, I wouldn't say
that adjusting the specializations to do the same thing with it is out
of scope.  This isn't unifying some completely unrelated aspect of v1
vs v2 and the resulting patch actually ended up being smaller.

Thanks,

                Ilya

On Wed, 2025-08-06 at 11:48 +0200, Max Kellermann wrote:
> This moves the list_empty() checks from the two callers (v1 and v2)
> into the base messenger.c library.  Now the v1/v2 specializations do
> not need to know about con->out_queue; that implementation detail is
> now hidden behind the ceph_con_get_out_msg() function.
> 
> Signed-off-by: Max Kellermann <max.kellermann@ionos.com>

Looks good.

Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>

Thanks,
Slava.

> ---
>  net/ceph/messenger.c    |  4 +++-
>  net/ceph/messenger_v1.c | 15 ++++++++++-----
>  net/ceph/messenger_v2.c |  4 ++--
>  3 files changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
> index 424fb2769b71..8886c38a55d2 100644
> --- a/net/ceph/messenger.c
> +++ b/net/ceph/messenger.c
> @@ -2113,7 +2113,9 @@ struct ceph_msg *ceph_con_get_out_msg(struct ceph_connection *con)
>  {
>  	struct ceph_msg *msg;
>  
> -	BUG_ON(list_empty(&con->out_queue));
> +	if (list_empty(&con->out_queue))
> +		return NULL;
> +
>  	msg = list_first_entry(&con->out_queue, struct ceph_msg, list_head);
>  	WARN_ON(msg->con != con);
>  
> diff --git a/net/ceph/messenger_v1.c b/net/ceph/messenger_v1.c
> index 516f2eeb122a..5eb6cfdbc494 100644
> --- a/net/ceph/messenger_v1.c
> +++ b/net/ceph/messenger_v1.c
> @@ -189,12 +189,18 @@ static void prepare_write_message_footer(struct ceph_connection *con, struct cep
>  
>  /*
>   * Prepare headers for the next outgoing message.
> + *
> + * @return false if there are no outgoing messages
>   */
> -static void prepare_write_message(struct ceph_connection *con)
> +static bool prepare_write_message(struct ceph_connection *con)
>  {
>  	struct ceph_msg *m;
>  	u32 crc;
>  
> +	m = ceph_con_get_out_msg(con);
> +	if (m == NULL)
> +		return false;
> +
>  	con_out_kvec_reset(con);
>  	con->v1.out_msg_done = false;
>  
> @@ -208,8 +214,6 @@ static void prepare_write_message(struct ceph_connection *con)
>  			&con->v1.out_temp_ack);
>  	}
>  
> -	m = ceph_con_get_out_msg(con);
> -
>  	dout("prepare_write_message %p seq %lld type %d len %d+%d+%zd\n",
>  	     m, con->out_seq, le16_to_cpu(m->hdr.type),
>  	     le32_to_cpu(m->hdr.front_len), le32_to_cpu(m->hdr.middle_len),
> @@ -256,6 +260,8 @@ static void prepare_write_message(struct ceph_connection *con)
>  	}
>  
>  	ceph_con_flag_set(con, CEPH_CON_F_WRITE_PENDING);
> +
> +	return true;
>  }
>  
>  /*
> @@ -1543,8 +1549,7 @@ int ceph_con_v1_try_write(struct ceph_connection *con)
>  			goto more;
>  		}
>  		/* is anything else pending? */
> -		if (!list_empty(&con->out_queue)) {
> -			prepare_write_message(con);
> +		if (prepare_write_message(con)) {
>  			goto more;
>  		}
>  		if (con->in_seq > con->in_seq_acked) {
> diff --git a/net/ceph/messenger_v2.c b/net/ceph/messenger_v2.c
> index 90109fa0fe60..e0b5f2e2582d 100644
> --- a/net/ceph/messenger_v2.c
> +++ b/net/ceph/messenger_v2.c
> @@ -3292,6 +3292,7 @@ static void finish_message(struct ceph_connection *con)
>  
>  static int populate_out_iter(struct ceph_connection *con)
>  {
> +	struct ceph_msg *msg;
>  	int ret;
>  
>  	dout("%s con %p state %d out_state %d\n", __func__, con, con->state,
> @@ -3337,8 +3338,7 @@ static int populate_out_iter(struct ceph_connection *con)
>  			pr_err("prepare_keepalive2 failed: %d\n", ret);
>  			return ret;
>  		}
> -	} else if (!list_empty(&con->out_queue)) {
> -		struct ceph_msg *msg = ceph_con_get_out_msg(con);
> +	} else if ((msg = ceph_con_get_out_msg(con)) != NULL) {
>  		ret = prepare_message(con, msg);
>  		if (ret) {
>  			pr_err("prepare_message failed: %d\n", ret);

On Fri, 2025-08-08 at 17:41 +0000, Viacheslav Dubeyko wrote:
> On Wed, 2025-08-06 at 11:48 +0200, Max Kellermann wrote:
> > This moves the list_empty() checks from the two callers (v1 and v2)
> > into the base messenger.c library.  Now the v1/v2 specializations do
> > not need to know about con->out_queue; that implementation detail is
> > now hidden behind the ceph_con_get_out_msg() function.
> > 
> > Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
> 
> Looks good.
> 
> Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> 
> 

Tested-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>

Thanks,
Slava.