Re: (ITS#8976) LMDB > use posix_madvise(3) instead of madvise(3) for SmartOS

[matthieu.guegan@smile-suisse.com]

On Fri, Feb 15, 2019 at 5:16 PM Howard Chu <hyc@symas.com> wrote:
>
> matthieu.guegan@smile-suisse.com wrote:
> > On Fri, Feb 15, 2019 at 4:33 PM Howard Chu <hyc@symas.com> wrote:
> >>
> >> Matthieu GUEGAN wrote:
> >>> On Fri, Feb 15, 2019 at 3:56 PM Howard Chu <hyc@symas.com> wrote:
> >>>>
> >>>> mguegan@virtua.ch wrote:
> >>>>> Full_Name: Matthieu Guegan
> >>>>> Version: current
> >>>>> OS: SmartOS
> >>>>> URL:
> >>>>> Submission from: (NULL) (109.190.148.77)
> >>>>>
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> In order to compile knot[1] on SmartOS, I have done a series of little patches.
> >>>>> One of them is linked to the LMDB project, which redirects me here.
> >>>>>
> >>>>> The idea is to tell SmartOS (and I think the SunOS family) to make use of
> >>>>> posix_madvise(3), instead of madvise(3) as implementation on this OS[2] is
> >>>>> different than, for example, the Linux[3] one.
> >>>>>
> >>>>> So, I kindly ask you to take a look at the merge request of the knot project[4],
> >>>>> in particular the `src/contrib/lmdb/mdb.c` patch (that's just a ifdef
> >>>>> modification) : would it be possible to apply this patch directly in this
> >>>>> repository ?
> >>>>
> >>>> I see no functional difference between your references [2] and [3].
> >>>> What is the benefit of this patch?
> >>>
> >>> Well, the Solaris/SunOS's madvise(3) is defined with an old `caddr_t`
> >>> type whereas Linux (and *BSD OSes too) use a more modern approach with
> >>> `void *`.
> >>> On the `knot` project, which embed the lmdb part of openldap,
> >>> compilation failed when using madvise(3) on SmartOS, so I did a
> >>> replacement with posix_madvise(3) which uses the `void *` argument
> >>> too.
> >>> I don't know what's the best way to handle this case, that's why I ask here.
> >>
> >> A caddr_t is just a typedef of "char *". It should always be legal to
> >> pass a void * to such a parameter. What is your compile error message?
> >>
> >
> > ```
> > contrib/lmdb/mdb.c: In function 'mdb_env_map':
> > contrib/lmdb/mdb.c:3998:3: error: implicit declaration of function
> > 'madvise'; did you mean 'raise'?
> > [-Werror=implicit-function-declaration]
> >    madvise(env->me_map, env->me_mapsize, MADV_RANDOM);
> >    ^~~~~~~
> >    raise
>
> This has absolutely nothing to do with whether the parameter is a caddr_t or a void *.
> Why would you even mention something totally irrelevant to the actual error?

Sorry if I mislead you with that, that was not my intention.
I was just trying to answer you when asking the difference using
posix_madvise() vs madvise() on SmartOS.
And that was the only thing I found. Indeed, it seems completely
irrelevant to the error I get.

> Strange that you have MADV_RANDOM defined but no declaration of madvise().

Indeed, searching in the header file `sys/mman.h` [5], it seems to be
possible to have a condition that enable madvise() and not MADV_RANDOM
or vice-versa.

> Sounds like your system's header files are playing stupid games with definitions.
> If you can find out what flags must be used to make the header files properly
> declare the madvise function, a patch for that may be acceptable.

I should say that I'm using the pkgsrc framework in order to compile
the whole thing and found multiple packages on which was applied the
same type of patch (excluding __sun when trying to use madvise(), thus
using the posix_madvise variant).
Flags can be overridden here, so it must be something related with the
pkgsrc's Makefile variables.

Anyway, maybe this should be done this way, instead of trying to
change things upstream. Thanks for your help.

[5] https://github.com/illumos/illumos-gate/blob/4e0c5eff9af325c80994e9527b7cb8b3a1ffd1d4/usr/src/uts/common/sys/mman.h