[dpdk-dev] [PATCH v6 04/11] eal/mem: extract common code for memseg list initialization

[Burakov, Anatoly]

On 10-Jun-20 5:39 PM, Dmitry Kozlyuk wrote:
> On Wed, 10 Jun 2020 16:48:58 +0100
> "Burakov, Anatoly" <anatoly.burakov at intel.com> wrote:
> 
>> On 10-Jun-20 3:31 PM, Dmitry Kozlyuk wrote:
>>> On Wed, 10 Jun 2020 11:26:22 +0100
>>> "Burakov, Anatoly" <anatoly.burakov at intel.com> wrote:
>>>
>>> [snip]
>>>>>>> +	addr = eal_get_virtual_area(
>>>>>>> +		msl->base_va, &mem_sz, page_sz, 0, reserve_flags);
>>>>>>> +	if (addr == NULL) {
>>>>>>> +#ifndef RTE_EXEC_ENV_WINDOWS
>>>>>>> +		/* The hint would be misleading on Windows, but this function
>>>>>>> +		 * is called from many places, including common code,
>>>>>>> +		 * so don't duplicate the message.
>>>>>>> +		 */
>>>>>>> +		if (rte_errno == EADDRNOTAVAIL)
>>>>>>> +			RTE_LOG(ERR, EAL, "Cannot reserve %llu bytes at [%p] - "
>>>>>>> +				"please use '--" OPT_BASE_VIRTADDR "' option\n",
>>>>>>> +				(unsigned long long)mem_sz, msl->base_va);
>>>>>>> +		else
>>>>>>> +			RTE_LOG(ERR, EAL, "Cannot reserve memory\n");
>>>>>>> +#endif
>>>>>>
>>>>>> You're left without any error messages on Windows. How about:
>>>>>>
>>>>>> const char *err_str = "Cannot reserve memory\n";
>>>>>> #ifndef RTE_EXEC_ENV_WINDOWS
>>>>>> if (rte_errno == EADDRNOTAVAIL)
>>>>>>        err_str = ...
>>>>>> #endif
>>>>>> RTE_LOG(ERR, EAL, err_str);
>>>>>>
>>>>>> or something like that?
>>>>>>      
>>>>>
>>>>> How about removing generic error message here completely and printing more
>>>>> specific messages at call sites? In fact, almost all of them already do this.
>>>>> It would be more helpful in tracking down errors.
>>>>>       
>>>>
>>>> Agreed, let's do that :) We do pass up the rte_errno, correct? So, we
>>>> should be able to do that.
>>>
>>> Actually, callers don't need rte_errno, because we only have to distinguish
>>> EADDRNOTAVAIL here, and eal_get_virtual_area() already prints precise
>>> diagnostics at WARNING and ERR level. rte_errno is preserved, however.
>>>    
>>
>> Not sure i agree, we still need the "--base-virtaddr" hint, and we can
>> only do that from the caller (without #ifdef-ery here), so we do need
>> rte_errno for that.
> 
> Maybe we're talking about different things. The "--base-virtaddr" hint is
> printed from eal_memseg_list_alloc() on Unices for EADDRNOTAVAIL.
> This is handy to avoid duplicating the hint and to provide context, so let's
> keep it despite #ifndef.
> 
> Otherwise, a generic error is printed from the same function (mistakenly not
> on Windows in v6). This generic error adds nothing to eal_get_virtual_area()
> logs and also doesn't help to known which exact eal_memseg_list_alloc()
> failed. If instead callers printed their own messages, it would be clear
> which call failed and in which context. Generic error can than be removed,
> eal_memseg_list_alloc() code simplified. Callers can inspect rte_errno if they
> ever need it, but really they don't, because hint is printed by
> eal_memseg_list_alloc() and eal_get_virtual_area() prints even more precise
> logs. This is what I did in v8.
> 

Right, OK :)

-- 
Thanks,
Anatoly