Linux-NFS Archive on lore.kernel.org
 help / color / Atom feed
From: Chuck Lever <chuck.lever@oracle.com>
To: Neil Brown <neilb@suse.de>,
	Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
	kernel-janitors@vger.kernel.org
Subject: Re: [PATCH 2/2] SUNRPC: Optimize 'svc_print_xprts()'
Date: Thu, 26 Mar 2020 18:29:41 -0400
Message-ID: <9AC34F25-68B7-4184-9A91-A6D026F6BA6F@oracle.com> (raw)
In-Reply-To: <87r1xe7pvy.fsf@notabene.neil.brown.name>



> On Mar 26, 2020, at 5:44 PM, NeilBrown <neilb@suse.de> wrote:
> 
> On Thu, Mar 26 2020, Christophe JAILLET wrote:
> 
>> Le 25/03/2020 à 23:53, NeilBrown a écrit :
>>> Can I suggest something more like this:
>>> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
>>> index de3c077733a7..0292f45b70f6 100644
>>> --- a/net/sunrpc/svc_xprt.c
>>> +++ b/net/sunrpc/svc_xprt.c
>>> @@ -115,16 +115,9 @@ int svc_print_xprts(char *buf, int maxlen)
>>>  	buf[0] = '\0';
>>> 
>>>  	spin_lock(&svc_xprt_class_lock);
>>> -	list_for_each_entry(xcl, &svc_xprt_class_list, xcl_list) {
>>> -		int slen;
>>> -
>>> -		sprintf(tmpstr, "%s %d\n", xcl->xcl_name, xcl->xcl_max_payload);
>>> -		slen = strlen(tmpstr);
>>> -		if (len + slen > maxlen)
>>> -			break;
>>> -		len += slen;
>>> -		strcat(buf, tmpstr);
>>> -	}
>>> +	list_for_each_entry(xcl, &svc_xprt_class_list, xcl_list)
>>> +		len += scnprintf(buf + len, maxlen - len, "%s %d\n",
>>> +				 xcl->xcl_name, xcl->xcl_max_payload);
>>>  	spin_unlock(&svc_xprt_class_lock);
>>> 
>>>  	return len;
>>> 
>>> NeilBrown
>> 
>> Hi,
>> 
>> this was what I suggested in the patch:
>>     ---
>>     This patch should have no functional change.
>>     We could go further, use scnprintf and write directly in the 
>> destination
>>     buffer. However, this could lead to a truncated last line.
>>     ---
> 
> Sorry - I missed that.
> So add
> 
> end = strrchr(tmpstr, '\n');
> if (end)
>    end[1] = 0;
> else
>    tmpstr[0] = 0;
> 
> or maybe something like
> 	list_for_each_entry(xcl, &svc_xprt_class_list, xcl_list) {
> 		int l = snprintf(buf + len, maxlen - len, "%s %d\n",
> 				 xcl->xcl_name, xcl->xcl_max_payload);
>                if (l < maxlen - len)
>                	len += l;
>        }
>        buf[len] = 0;
> 
> There really is no need to have the secondary buffer, and I think doing
> so just complicates the code.

In the interest of getting this fix into the upcoming merge window, let's
stick with the temporary buffer approach. Thanks!


--
Chuck Lever




      reply index

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-25  7:04 Christophe JAILLET
2020-03-25 14:52 ` Chuck Lever
     [not found]   ` <42afbf1f-19e1-a05c-e70c-1d46eaba3a71@wanadoo.fr>
2020-03-25 22:53     ` NeilBrown
     [not found]       ` <2e2d1293-c978-3f1d-5a1e-dc43dc2ad06b@wanadoo.fr>
2020-03-26 21:44         ` NeilBrown
2020-03-26 22:29           ` Chuck Lever [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=9AC34F25-68B7-4184-9A91-A6D026F6BA6F@oracle.com \
    --to=chuck.lever@oracle.com \
    --cc=christophe.jaillet@wanadoo.fr \
    --cc=kernel-janitors@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=neilb@suse.de \
    /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

Linux-NFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nfs/0 linux-nfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nfs linux-nfs/ https://lore.kernel.org/linux-nfs \
		linux-nfs@vger.kernel.org
	public-inbox-index linux-nfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-nfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git