On Wed, 10 Jun 2020 16:48:58 +0100
"Burakov, Anatoly" <anatoly.bura...@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.bura...@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.
--
Dmitry Kozlyuk
