[PATCH] Remove "multicasting" inside messages.c

[Jeremy Allison]

On Tue, Jun 20, 2017 at 10:10:33PM +0200, Volker Lendecke via samba-technical wrote:
> Hi!
> 
> There's more to come. g_lock_ping_pong (like ctdb's ping_pong on
> g_lock) is a fun test to run. It exposes quite a few problems in our
> messaging system.
> 
> Until then, review appreciated!

W00t! Looking forward to reviewing g_lock_ping_pong !

Until then, RB+. Pushed.

> SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
> phone: +49-551-370000-0, fax: +49-551-370000-9
> AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
> http://www.sernet.de, mailto:kontakt at sernet.de

> From 2cbe1ccd9177116dda52dc014b3cc192a5d63a13 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Sat, 17 Jun 2017 08:48:16 +0200
> Subject: [PATCH] messaging: Deliver messages only once
> 
> This survived an autobuild, so no subsystem strictly needs this anymore. In
> particular the notify subsystem has been rewritten.
> 
> Why this patch? It removes some complexity from core code, and it reduces the
> potential memory overconsumption: Right now I'm working on a g_lock_ping_pong
> test. This test does a lot of messaging_filtered_read_send calls in a tight
> loop on a nested event context. With the current code we let the
> messaging_filtered_read code consume the message that arrives, but it also
> posts it for consumption by the main event context attached to the messaging
> context with its "classic" callback. This test never comes back to the main
> event context, so it accumulates more and more self-posted messages. That's
> just unnecessary, given that due to the successful autobuild nothing but the
> read1 test makes use of the "multicasting" of messages.
> 
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
>  source3/lib/messages.c                | 24 ++++++++++--------------
>  source3/torture/test_messaging_read.c |  6 +++---
>  2 files changed, 13 insertions(+), 17 deletions(-)
> 
> diff --git a/source3/lib/messages.c b/source3/lib/messages.c
> index a38f40e..b0edb30 100644
> --- a/source3/lib/messages.c
> +++ b/source3/lib/messages.c
> @@ -1015,7 +1015,7 @@ static bool messaging_append_new_waiters(struct messaging_context *msg_ctx)
>  	return true;
>  }
>  
> -static void messaging_dispatch_classic(struct messaging_context *msg_ctx,
> +static bool messaging_dispatch_classic(struct messaging_context *msg_ctx,
>  				       struct messaging_rec *rec)
>  {
>  	struct messaging_callback *cb, *next;
> @@ -1041,13 +1041,10 @@ static void messaging_dispatch_classic(struct messaging_context *msg_ctx,
>  		cb->fn(msg_ctx, cb->private_data, rec->msg_type,
>  		       rec->src, &rec->buf);
>  
> -		/*
> -		 * we continue looking for matching messages after finding
> -		 * one. This matters for subsystems like the internal notify
> -		 * code which register more than one handler for the same
> -		 * message type
> -		 */
> +		return true;
>  	}
> +
> +	return false;
>  }
>  
>  /*
> @@ -1058,9 +1055,13 @@ static void messaging_dispatch_rec(struct messaging_context *msg_ctx,
>  				   struct messaging_rec *rec)
>  {
>  	size_t i;
> +	bool consumed;
>  
>  	if (ev == msg_ctx->event_ctx) {
> -		messaging_dispatch_classic(msg_ctx, rec);
> +		consumed = messaging_dispatch_classic(msg_ctx, rec);
> +		if (consumed) {
> +			return;
> +		}
>  	}
>  
>  	if (!messaging_append_new_waiters(msg_ctx)) {
> @@ -1102,12 +1103,7 @@ static void messaging_dispatch_rec(struct messaging_context *msg_ctx,
>  		if ((ev == state->ev) &&
>  		    state->filter(rec, state->private_data)) {
>  			messaging_filtered_read_done(req, rec);
> -
> -			/*
> -			 * Only the first one gets the fd-array
> -			 */
> -			rec->num_fds = 0;
> -			rec->fds = NULL;
> +			return;
>  		}
>  
>  		i += 1;
> diff --git a/source3/torture/test_messaging_read.c b/source3/torture/test_messaging_read.c
> index 43b4367..d3e4079 100644
> --- a/source3/torture/test_messaging_read.c
> +++ b/source3/torture/test_messaging_read.c
> @@ -122,7 +122,7 @@ bool run_messaging_read1(int dummy)
>  		goto fail;
>  	}
>  
> -	for (i=0; i<3; i++) {
> +	for (i=0; i<2; i++) {
>  		if (tevent_loop_once(ev) != 0) {
>  			fprintf(stderr, "tevent_loop_once failed\n");
>  			goto fail;
> @@ -131,8 +131,8 @@ bool run_messaging_read1(int dummy)
>  
>  	printf("%u/%u\n", count1, count2);
>  
> -	if ((count1 != 1) || (count2 != 1)){
> -		fprintf(stderr, "Got %u/%u msgs, expected 1 each\n",
> +	if ((count1 != 1) || (count2 != 0)) {
> +		fprintf(stderr, "Got %u/%u msgs, expected 1/0\n",
>  			count1, count2);
>  		goto fail;
>  	}
> -- 
> 2.1.4
>