netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Karsten Graul <kgraul@linux.ibm.com>
To: Tony Lu <tonylu@linux.alibaba.com>
Cc: kuba@kernel.org, davem@davemloft.net, netdev@vger.kernel.org,
	linux-s390@vger.kernel.org
Subject: Re: [PATCH net-next] net/smc: Use kvzalloc for allocating smc_link_group
Date: Thu, 27 Jan 2022 16:28:38 +0100	[thread overview]
Message-ID: <8f56ed63-79b4-2a75-4e95-f4f6a0ad8430@linux.ibm.com> (raw)
In-Reply-To: <Ye51fyoWrXTevmLa@TonyMac-Alibaba>

On 24/01/2022 10:46, Tony Lu wrote:
> On Fri, Jan 21, 2022 at 12:06:56PM +0100, Karsten Graul wrote:
>> On 21/01/2022 04:24, Tony Lu wrote:
>>> On Thu, Jan 20, 2022 at 03:50:26PM +0100, Karsten Graul wrote:
>>>> On 20/01/2022 15:09, Tony Lu wrote:
>> I am still not fully convinced of this change. It does not harm and the overhead of
>> a vmalloc() is acceptable because a link group is not created so often. But since
>> kvzmalloc() will first try to use normal kmalloc() and if that fails switch to the
>> (more expensive) vmalloc() this will not _save_ any contiguous memory.
>> And for the subsequent required allocations of at least one RMB we need another 16KB.
> I agree with you. kvzmalloc doesn't save contiguous memory for the most
> time, only when high order contiguous memory is used out, or malloc link
> group when another link group just freed its buffer. This race window is
> too small to reach it in real world.

Okay I see we are in sync with that, and we should drop your kvzalloc() patch.
It generates noise and doesn't solve a real problem.

I appreciate your work on this topic, but when I see the numbers then the whole lgr 
WITH all links inside it would occupy less than one 4K page of memory (~3808 bytes).
The vast majority of memory in this struct is needed by the

struct smc_rtoken rtokens[255][3]; /*  3472 12240 */

array. This is where continuous space could be saved, but that needs some effort
to provide an equivalent fast way to store and lookup the RMBs.

Moving out the links from the lgr will not help here.

A link group holds up to 255 connections, so even with your 10000 connection test
we need no more than 40 instances of lgr...I am not sure if it is worth the time that
you need to spend for this particular change (lgr). The other topics you listed also
sound interesting!

      reply	other threads:[~2022-01-27 15:28 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-20 14:09 [PATCH net-next] net/smc: Use kvzalloc for allocating smc_link_group Tony Lu
2022-01-20 14:50 ` Karsten Graul
2022-01-21  3:24   ` Tony Lu
2022-01-21 11:06     ` Karsten Graul
2022-01-24  9:46       ` Tony Lu
2022-01-27 15:28         ` Karsten Graul [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8f56ed63-79b4-2a75-4e95-f4f6a0ad8430@linux.ibm.com \
    --to=kgraul@linux.ibm.com \
    --cc=davem@davemloft.net \
    --cc=kuba@kernel.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=tonylu@linux.alibaba.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).