Re: git: 6260bfb08742 - main - powerpc: Optimize copyinstr() to avoid repeatedly mapping user strings

[Brandon Bergren]

Approved-By: bdragon (in IRC)

On Wed, Dec 30, 2020, at 4:45 PM, Piotr Kubaj wrote:
> The branch main has been updated by pkubaj (ports committer):
> 
> URL: 
> https://cgit.FreeBSD.org/src/commit/?id=6260bfb0874280d3fb58ac52699e2aee6ecca8a8
> 
> commit 6260bfb0874280d3fb58ac52699e2aee6ecca8a8
> Author:     Justin Hibbits <chmeeedalf at gmail.com>
> AuthorDate: 2020-06-04 18:15:15 +0000
> Commit:     Piotr Kubaj <pkubaj at FreeBSD.org>
> CommitDate: 2020-12-30 22:45:35 +0000
> 
>     powerpc: Optimize copyinstr() to avoid repeatedly mapping user strings
>     
>     Currently copyinstr() uses fubyte() to read each byte from userspace.
>     However, this means that for each byte, it calls pmap_map_user_ptr() to
>     map the string into memory.  This is needlessly wasteful, since the
>     string will rarely ever cross a segment boundary.  Instead, map a
>     segment at a time, and copy as much from that segment as possible at a
>     time.
>     
>     Measured with the HPT pmap on powerpc64, this saves roughly 8% time on
>     buildkernel, and 5% on buildworld, in wallclock time.
> ---
>  sys/powerpc/powerpc/copyinout.c | 46 +++++++++++++++++++++++++++++------------
>  1 file changed, 33 insertions(+), 13 deletions(-)
> 
> diff --git a/sys/powerpc/powerpc/copyinout.c b/sys/powerpc/powerpc/copyinout.c
> index 76965ad996b8..1528accc0e0e 100644
> --- a/sys/powerpc/powerpc/copyinout.c
> +++ b/sys/powerpc/powerpc/copyinout.c
> @@ -236,31 +236,51 @@ REMAP(copyin)(const void *udaddr, void *kaddr, size_t len)
>  int
>  REMAP(copyinstr)(const void *udaddr, void *kaddr, size_t len, size_t *done)
>  {
> +	struct		thread *td;
> +	pmap_t		pm;
> +	jmp_buf		env;
>  	const char	*up;
> -	char		*kp;
> -	size_t		l;
> -	int		rv, c;
> +	char		*kp, *p;
> +	size_t		i, l, t;
> +	int		rv;
>  
> -	kp = kaddr;
> -	up = udaddr;
> +	td = curthread;
> +	pm = &td->td_proc->p_vmspace->vm_pmap;
>  
> +	t = 0;
>  	rv = ENAMETOOLONG;
>  
> -	for (l = 0; len-- > 0; l++) {
> -		if ((c = fubyte(up++)) < 0) {
> +	td->td_pcb->pcb_onfault = &env;
> +	if (setjmp(env)) {
> +		rv = EFAULT;
> +		goto done;
> +	}
> +
> +	kp = kaddr;
> +	up = udaddr;
> +
> +	while (len > 0) {
> +		if (pmap_map_user_ptr(pm, up, (void **)&p, len, &l)) {
>  			rv = EFAULT;
> -			break;
> +			goto done;
>  		}
>  
> -		if (!(*kp++ = c)) {
> -			l++;
> -			rv = 0;
> -			break;
> +		for (i = 0; len > 0 && i < l; i++, t++, len--) {
> +			if ((*kp++ = *p++) == 0) {
> +				i++, t++;
> +				rv = 0;
> +				goto done;
> +			}
>  		}
> +
> +		up += l;
>  	}
>  
> +done:
> +	td->td_pcb->pcb_onfault = NULL;
> +
>  	if (done != NULL) {
> -		*done = l;
> +		*done = t;
>  	}
>  
>  	return (rv);
>

-- 
  Brandon Bergren
  bdragon at FreeBSD.org