From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Burakov, Anatoly" <anatoly.burakov@intel.com>
Subject: Re: [PATCH 4/6] mem: use address hint for mapping
	hugepages
Date: Tue, 3 Jul 2018 10:17:43 +0100
Message-ID: <adefbe83-1c28-a84b-fc70-4efd886a3333@intel.com>
References: <1530552423-32301-1-git-send-email-alejandro.lucero@netronome.com>
 <1530552423-32301-5-git-send-email-alejandro.lucero@netronome.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Cc: stable@dpdk.org, maxime.coquelin@redhat.com
To: Alejandro Lucero <alejandro.lucero@netronome.com>, dev@dpdk.org
Return-path: <dev-bounces@dpdk.org>
In-Reply-To: <1530552423-32301-5-git-send-email-alejandro.lucero@netronome.com>
Content-Language: en-US
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

On 02-Jul-18 6:27 PM, Alejandro Lucero wrote:
> Linux kernel uses a really high address as starting address for
> serving mmaps calls. If there exists addressing limitations and
> IOVA mode is VA, this starting address is likely too high for
> those devices. However, it is possible to use a lower address in
> the process virtual address space as with 64 bits there is a lot
> of available space.
> 
> This patch adds an address hint as starting address for 64 bits
> systems.
> 
> Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> ---

<snip>

>   	long aligned_addr;
>   
> -	if (internal_config.base_virtaddr != 0) {
> -		addr = (void*) (uintptr_t) (internal_config.base_virtaddr +
> -				baseaddr_offset);
> -	}
> -	else addr = NULL;
> -
>   	RTE_LOG(DEBUG, EAL, "Ask a virtual area of 0x%zx bytes\n", *size);
>   
>   	fd = open("/dev/zero", O_RDONLY);
> @@ -278,7 +289,22 @@
>   		return NULL;
>   	}
>   	do {
> -		addr = mmap(addr,
> +		if (internal_config.base_virtaddr != 0) {
> +			addr_hint = (void *) (uintptr_t)
> +				    (internal_config.base_virtaddr +
> +				     baseaddr_offset);
> +		}
> +#ifdef RTE_ARCH_64
> +		else {
> +			addr_hint = (void *) (uintptr_t) (baseaddr +
> +					baseaddr_offset);
> +		}
> +#else
> +		else {
> +			addr_hint = NULL;
> +		}
> +#endif

If my understanding is correct, calculations are all done on static 
variables, only the result is assigned to addr_hint which is local. Can 
we move this into a function and put these #ifdefs there, while keeping 
this code clean?

> +		addr = mmap(addr_hint,
>   				(*size) + hugepage_sz, PROT_READ,
>   #ifdef RTE_ARCH_PPC_64
>   				MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB,
> @@ -286,8 +312,14 @@
>   				MAP_PRIVATE,
>   #endif
>   				fd, 0);
> -		if (addr == MAP_FAILED)
> +		if (addr == MAP_FAILED) {
> +			/* map failed. Let's try with less memory */
>   			*size -= hugepage_sz;
> +		} else if (addr_hint && addr != addr_hint) {
> +			/* map not using hint. Let's try with another offset */

Comment is slightly misleading - "map not using hint" implies we are 
about to map something without using hint. Suggested rewording:

suggested address hint was not used, try with another offset

> +			addr = MAP_FAILED;
> +			baseaddr_offset += 0x100000000;
> +		}
>   	} while (addr == MAP_FAILED && *size > 0);
>   
>   	if (addr == MAP_FAILED) {
> 


-- 
Thanks,
Anatoly