Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 22 Dec 2022 12:54:35 -0500
From:   Brian Foster <bfoster@redhat.com>
To:     Roman Gushchin <roman.gushchin@linux.dev>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Shakeel Butt <shakeelb@google.com>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Michal Hocko <mhocko@kernel.org>,
        Muchun Song <muchun.song@linux.dev>,
        Andrew Morton <akpm@linux-foundation.org>,
        Waiman Long <longman@redhat.com>,
        Sven Luther <Sven.Luther@windriver.com>
Subject: Re: [PATCH RFC] ipc/mqueue: introduce msg cache
Message-ID: <Y6SZ28uWzWqaSik2@bfoster>
References: <20221220184813.1908318-1-roman.gushchin@linux.dev>
 <Y6RE5iUHSinUJxDt@bfoster>
 <Y6SHzFPJLEr09JHF@P9FQF9L96D>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y6SHzFPJLEr09JHF@P9FQF9L96D>
Precedence: bulk

On Thu, Dec 22, 2022 at 08:37:32AM -0800, Roman Gushchin wrote:
> On Thu, Dec 22, 2022 at 06:52:06AM -0500, Brian Foster wrote:
> > On Tue, Dec 20, 2022 at 10:48:13AM -0800, Roman Gushchin wrote:
> > > Sven Luther reported a regression in the posix message queues
> > > performance caused by switching to the per-object tracking of
> > > slab objects introduced by patch series ending with the
> > > commit 10befea91b61 ("mm: memcg/slab: use a single set of kmem_caches for all
> > > allocations").
> > > 
> > > To mitigate the regression cache allocated mqueue messages on a small
> > > percpu cache instead of releasing and re-allocating them every time.
> > > 
> > > This change brings the performance measured by a benchmark kindly
> > > provided by Sven [1] almost back to the original (or even better)
> > > numbers. Measurements results are also provided by Sven.
> > > 
> > > +------------+---------------------------------+--------------------------------+
> > > | kernel     | mqueue_rcv (ns)     variation   | mqueue_send (ns)   variation   |
> > > | version    | min avg  max      min      avg  | min avg max       min     avg  |
> > > +------------+--------------------------+---------------------------------------+
> > > | 4.18.45    | 351 382 17533    base     base  | 383 410 13178     base    base |
> > > | 5.8-good   | 380 392  7156   -7,63%  -2,55%  | 376 384  6225    1,86%   6,77% |
> > > | 5.8-bad    | 524 530  5310  -33,02% -27,92%  | 512 519  8775  -25,20% -21,00% |
> > > | 5.10       | 520 533  4078  -32,20% -28,33%  | 518 534  8108  -26,06% -23,22% |
> > > | 5.15       | 431 444  8440  -18,56% -13,96%  | 425 437  6170   -9,88%  -6,18% |
> > > | 6.0.3      | 474 614  3881  -25,95% -37,79%  | 482 693   931  -20,54% -40,84% |
> > > | 6.1-rc8    | 496 509  8804  -29,23% -24,95%  | 493 512  5748  -22,31% -19,92% |
> > > | 6.1-rc8+p  | 392 397  5479  -10,46%  -3,78%  | 364 369 10776    5,22%  11,11% |
> > > +------------+---------------------------------+--------------------------------+
> > > 
> > > The last line reflects the result with this patch ("6.1-rc8+p").
> > > 
> > > [1]: https://lore.kernel.org/linux-mm/Y46lqCToUa%2FBgt%2Fc@P9FQF9L96D/T/
> > > 
> > > Reported-by: Sven Luther <Sven.Luther@windriver.com>
> > > Tested-by: Sven Luther <Sven.Luther@windriver.com>
> > > Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
> > > ---
> > >  include/linux/memcontrol.h |  12 +++++
> > >  ipc/mqueue.c               |  20 ++++++--
> > >  ipc/msg.c                  |  12 ++---
> > >  ipc/msgutil.c              | 101 +++++++++++++++++++++++++++++++++----
> > >  ipc/util.h                 |   8 ++-
> > >  mm/memcontrol.c            |  62 +++++++++++++++++++++++
> > >  6 files changed, 194 insertions(+), 21 deletions(-)
> > > 
> > ...
> > > diff --git a/ipc/msgutil.c b/ipc/msgutil.c
> > > index d0a0e877cadd..8667708fc00a 100644
> > > --- a/ipc/msgutil.c
> > > +++ b/ipc/msgutil.c
> > ...
> > > @@ -39,16 +40,76 @@ struct msg_msgseg {
> > ...
> > > +static struct msg_msg *alloc_msg(size_t len, struct msg_cache *cache)
> > >  {
> > >  	struct msg_msg *msg;
> > >  	struct msg_msgseg **pseg;
> > >  	size_t alen;
> > >  
> > > +	if (cache) {
> > > +		struct pcpu_msg_cache *pc;
> > > +
> > > +		msg = NULL;
> > > +		pc = get_cpu_ptr(cache->pcpu_cache);
> > > +		if (pc->msg && pc->len == len) {
> > > +			struct obj_cgroup *objcg;
> > > +
> > > +			rcu_read_lock();
> > > +			objcg = obj_cgroup_from_current();
> > > +			if (objcg == pc->objcg) {
> > > +				msg = pc->msg;
> > > +				pc->msg = NULL;
> > > +				obj_cgroup_put(pc->objcg);
> > > +			}
> > > +			rcu_read_unlock();
> > > +		}
> > > +		put_cpu_ptr(cache->pcpu_cache);
> > > +		if (msg)
> > > +			return msg;
> > > +	}
> > > +
> > >  	alen = min(len, DATALEN_MSG);
> > >  	msg = kmalloc(sizeof(*msg) + alen, GFP_KERNEL_ACCOUNT);
> > >  	if (msg == NULL)
> > > @@ -77,18 +138,19 @@ static struct msg_msg *alloc_msg(size_t len)
> > >  	return msg;
> > >  
> > >  out_err:
> > > -	free_msg(msg);
> > > +	free_msg(msg, NULL);
> > >  	return NULL;
> > >  }
> > >  
> > > -struct msg_msg *load_msg(const void __user *src, size_t len)
> > > +struct msg_msg *load_msg(const void __user *src, size_t len,
> > > +			 struct msg_cache *cache)
> > >  {
> > >  	struct msg_msg *msg;
> > >  	struct msg_msgseg *seg;
> > >  	int err = -EFAULT;
> > >  	size_t alen;
> > >  
> > > -	msg = alloc_msg(len);
> > > +	msg = alloc_msg(len, cache);
> > >  	if (msg == NULL)
> > >  		return ERR_PTR(-ENOMEM);
> > >  
> > > @@ -104,14 +166,16 @@ struct msg_msg *load_msg(const void __user *src, size_t len)
> > >  			goto out_err;
> > >  	}
> > >  
> > > -	err = security_msg_msg_alloc(msg);
> > > -	if (err)
> > > -		goto out_err;
> > > +	if (!msg->security) {
> > > +		err = security_msg_msg_alloc(msg);
> > > +		if (err)
> > > +			goto out_err;
> > > +	}
> > >  
> > >  	return msg;
> > >  
> > >  out_err:
> > > -	free_msg(msg);
> > > +	free_msg(msg, NULL);
> > >  	return ERR_PTR(err);
> > >  }
> > >  #ifdef CONFIG_CHECKPOINT_RESTORE
> > > @@ -166,10 +230,29 @@ int store_msg(void __user *dest, struct msg_msg *msg, size_t len)
> > >  	return 0;
> > >  }
> > >  
> > > -void free_msg(struct msg_msg *msg)
> > > +void free_msg(struct msg_msg *msg, struct msg_cache *cache)
> > >  {
> > >  	struct msg_msgseg *seg;
> > >  
> > > +	if (cache) {
> > > +		struct pcpu_msg_cache *pc;
> > > +		bool cached = false;
> > > +
> > > +		pc = get_cpu_ptr(cache->pcpu_cache);
> > > +		if (!pc->msg) {
> > > +			pc->objcg = get_obj_cgroup_from_slab_obj(msg);
> > > +			pc->len = msg->m_ts;
> > > +			pc->msg = msg;
> > > +
> > > +			if (pc->objcg)
> > > +				cached = true;
> > > +		}
> > 
> > Hi Roman,
> > 
> > It seems that this is kind of tailored to the ideal conditions
> > implemented by the test case: i.e., a single, fixed size message being
> > passed back and forth on a single cpu. Does that actually represent the
> > production workload?
> 
> Hi Brian!
> 
> Not really, it was all based on Sven's report and the benchmark he provided.
> I assume that the benchmark emulates the production workload he has, but
> it's up to him to confirm.
> 
> Personally I haven't seen a lot of mqueue usage in the production, especially
> for anything performance-sensitive. Also, Sven reported the regression which
> was introduced in 5.9, so I take it as an indicator that not so many users
> depend on the mqueue performance.
> 

Ok, thanks for the context. I've not either TBH. I guess even though
it's quite helpful to simplify a bug/regression into a test program, it
might be useful to confirm whatever improvement translates back to the
production workload if it happens to be more involved. If it's really as
simple as the test program, then carry on.. nothing to see here :). But
I'll defer to Sven..

> > 
> > I'm a little curious if/how this might work for workloads that might
> > involve more variable sized messages, deeper queue depths (i.e.  sending
> > more than one message before attempting a recv) and more tasks across
> > different cpus. For example, it looks like if an "uncommonly" sized
> > message ended up cached on a cpu, this would always result in subsequent
> > misses because the alloc side requires an exact size match and the free
> > side never replaces a cached msg. Hm?
> 
> Yes, of course it's very primitive. But I'm not sure we want to implement
> something complicated here. If there any specific ideas, I'm totally up for
> them. We can cache 2 sizes or 4 sizes or something else, but Idk how much value
> it has.
> 

Agreed on avoiding unnecessary complexity. FWIW, a couple thoughts that
crossed my mind when skimming this patch were to do something like
unconditionally replace a cached msg with one being freed (and hope for
some size repetition?), or perhaps always replace a cached msg with a
larger one being freed. Also if it is possible to use a larger
physically allocated msg to send a smaller actual message (?), perhaps
it might make sense to use the cached msg so long as it is large enough
for the current request (instead of exactly matching).

Any of those may or may not be more complex than just caching multiple
objects (i.e. may require to track physical msg size, or to realloc a
msg, etc.) or just may not add any measurable value over the current
approach. Just some random ideas. ;)

Brian

> Btw, in parallel I'm working on some generic improvements to the slab
> allocation path [1], maybe it will be good enough for Sven.
> 
> [1]: https://lore.kernel.org/linux-mm/20221220204451.gm5d3pdbfvd5ki6b@google.com/T/
> 
> Thanks!
>