nvml comments

[Jeff Moyer]

Hi, Andy,

I've taken a cursory look at the library, and here are my initial
comments.

I hesitate to start looking at or thinking about the transaction apis
without first catching up with you. I see that the github repo is
pretty recently updated, so that's promising. Is there any substantial
amount of work that you've done privately that hasn't yet gone into
github? I'm trying to figure out where I can help out, and, more
importantly, I don't want to waste time on things you've already done.

Cheers,
Jeff

General Comments:
----------------
Consider renaming nvml to libnvm. This is a more canonical naming scheme.

*_map() functions:

In general, I'm not sure why the map functions do not support all of
the arguments that mmap does. This makes things like extending the
file cumbersome.

The method for determining the amount of user data that can be stored
in a give type of pool is clumsy. nvml should either provide an api
that creates a map with the specified amount of user-available
blocks/data, or it should provide an api that will give a file size
based on the number of user-available blocks desired, which will then
be handed to fallocate by the application.

In general, I don't like how a file with no header is just automatically
destroyed. I would prefer to see either a separate api call for
creation, or a flag added to map for the purpose.

> From what I've seen, the endianness conversion is incomplete. If you're
just going to convert the header, then surely you should store the data
endianness in there! If you expect applications to convert all data put
into the log or transactions or data blocks to also be converted, I
think you'll hear a lot of groaning from developers. So, what was the
intention there?

> From the pmemblk_map documentation:
There are no restrictions on the block size bsize, however libp-
mem will silently round up the given size to PMEMBLK_MIN_BLK, as
defined in <libpmem.h>.

That sounds dangerous, especially given your read and write routines don't
take a length argument. I would *strongly* urge you to fail invocations
that do not have proper alignment.

Also consider adding some libpmem-specific error codes, especially for
things like the EINAL returned for a corrupted pool.

Why is the minimum block pool size 1GB?

size_t pmemblk_nblock(PMEMblk *pbp);

Again, from the docs:
The pmemblk_nblock() function returns the usable space in the
block memory pool, expressed as the number of blocks available.
pbp must be a block memory pool as returned by pmemblk_map().

It may be helpful to clarify that this is not the amount of free space.

For pmemlog, I had anticipated a ring buffer. Do you expect the users
of the pmemlog api to create multiple log files, and switch between them
as they fill up, with a background process pushing data out from full
log files? Had you considered a ring buffer for this?

In general, it would be helpful if you mentioned the intended design for
an application making use of these apis.

Notably missing is a persistent memory allocator. I'm guessing this will be
tied to the transaction api. Is that right?

Code Comments:
-------------

I also did some *extremely* cursory code review. Here are my comments,
for whatever they're worth:

log levels -- give them names/#defines

#define PROCMAXLEN 2048 /* maximum expected line length in /proc files */

How did you deduce that?

* util_map_hint -- (internal) use /proc to determine a hint address for mmap()
* - greater or equal 1TB,
why?

return (*Func_is_pmem)(addr, len);
no need for parens... return Func_is_pmem(addr, len);
Unless that's just your style? Maybe you wanted to call attention to
the fact that it's a function pointer?

#define PMEMTRN_MIN_POOL ((size_t)(1024 * 1024 * 2)) /* min pool size: 2MB */

comment tells what but not why... why 2MB?

log.h:
#define LOG_FORMAT_DATA_ALIGN 4096

I think this wants to be pagesize? If so, a configure script would be
able to generate a config.h with the appropriate value for the platform.

[Andy Rudoff]

Great comments, Jeff. I appreciate it! Some responses in-line below...

-andy

> I hesitate to start looking at or thinking about the transaction apis
> without first catching up with you. I see that the github repo is
> pretty recently updated, so that's promising. Is there any substantial
> amount of work that you've done privately that hasn't yet gone into
> github? I'm trying to figure out where I can help out, and, more
> importantly, I don't want to waste time on things you've already done.

The team is currently working on prototyping the pmemobj APIs and I'll
be posting details on that soon. But don't let that discourage you
from working on something. If you have an area you'd like to work on,
I suggest creating an issue in the issues database so we know you're
working on it. Hmm. Maybe I should do that for the stuff I'm working
on :-)

> Consider renaming nvml to libnvm. This is a more canonical naming scheme.

The project name is the NVM Library, but none of the deliverables are
named that. Currently everything builds as part of the volatile
library, libvmem or the persistent library, libpmem. There are some
other "nvm" libraries out there so I didn't try to re-use that name.

Now that we've written lots of examples and tests for the first few
sections of the API, we're cleaning up some naming and packaging this
week. So you'll see pushes to github that break it into these
libraries:

libvmem (Same as before.)
libpmem (Small, general support for flushing and raw pmem access.
Understands the new instructions.)
libpmemblk (Built on libpmem, provides the atomic array of blocks.)
libpmemlog (Built on libpmem, provides the pmem-resident log file.)
libpmemobj (Built on libpmem, provides the transactional object store.
This is the flexible transaction stuff.)

And in the future, we think we'll be building libraries for a
key-value store, pmem-aware hash tables and trees, etc. that are built
on libpmemobj.

Anyway, all that information was just to make it clear that "nvml" is
just the name of the project, but it is really a set of libraries,
named and packaged up in the usual way.

> *_map() functions:

As part of this week's cleanup, we're renaming some of the data types
based on some confusion we saw from people while explaining it, and
we're changing all the *_map() and *_unmap() functions to things like
pmemblk_pool_open() and pmemblk_pool_close(), operating on paths to
the files instead of the file descriptors. Yes, we still use mmap()
internally but that's an implementation detail that shouldn't be
reflected in the name.

> In general, I'm not sure why the map functions do not support all of
> the arguments that mmap does. This makes things like extending the
> file cumbersome.

pmem files are like file systems. They contain data structures that
live across crashes and need to be kept consistent. So you can't just
grow the file and expect the right thing to happen any more than you
can grow a partition and expect the file system to just work --
there's usually an administrative "grow" command if growing is
supported. That's the same strategy we're using here -- when growing
is supported there will be an API for it in the appropriate libraries
and an administrative tool will allow you to do it from the command
line. (The admin tools for these pools is being written and will be
pushed to github in a few weeks, I estimate.)

> The method for determining the amount of user data that can be stored
> in a give type of pool is clumsy. nvml should either provide an api
> that creates a map with the specified amount of user-available
> blocks/data, or it should provide an api that will give a file size
> based on the number of user-available blocks desired, which will then
> be handed to fallocate by the application.

Let me think about this one. Today when you create a file system, you
tell the system how much storage you're willing to commit to that file
system (by sizing the partition) and you get the usable amount of data
that the file system gives you after it creates its metadata. We're
following the same model. I've never seen a system where you say "I
want 500GB of usable space, create whatever size partition you need to
so ext4 gives me that." But it is an interesting idea. Let me ponder
it a bit...

> In general, I don't like how a file with no header is just automatically
> destroyed. I would prefer to see either a separate api call for
> creation, or a flag added to map for the purpose.

Good feedback, thanks.

> From what I've seen, the endianness conversion is incomplete. If you're
> just going to convert the header, then surely you should store the data
> endianness in there! If you expect applications to convert all data put
> into the log or transactions or data blocks to also be converted, I
> think you'll hear a lot of groaning from developers. So, what was the
> intention there?

Yes, but...

The answer is pool-specific. Some pool types, like pmemblk, are meant
to be portable across different machine types. So the (soon to be
published) specification for it states the exact format of all
metadata, namely that it must be little endian, specific field sizes,
etc. This is how most file systems work -- the metadata is made
portable, but the user data can be anything, either non-portable or
portable, depending on what the app writer decides to do. What would
storing the endianness in the header do in this case? Pmemblk is
always LE.

But for pmemobj (which you haven't seen yet), I'm specifically saying
it is not an interchange format in any way. Objects are stored in
native C format, including whatever padding the compiler puts in
structs, etc. so that programs can ask for pointers directly to the
data structures in persistent memory. I feel very strongly that this
allows applications to exploit the addressable nature of pmem and any
required marshalling for pmemopbj would make it no better than an API
sitting on storage. But you'll still see the pool header byte-swapped
and for pmemobj you will indeed see what you're expecting: we'll store
information about the platform where the pool was created.
Byte-swapping it allows us to open it and read our header, and then if
the current platform is incompatible with the creating platform, the
library can detect it and error out appropriately.

>
>> From the pmemblk_map documentation:
> There are no restrictions on the block size bsize, however libp-
> mem will silently round up the given size to PMEMBLK_MIN_BLK, as
> defined in <libpmem.h>.
>
> That sounds dangerous, especially given your read and write routines don't
> take a length argument. I would *strongly* urge you to fail invocations
> that do not have proper alignment.

Nothing to do with alignment. The text you quoted is just saying that
if you ask for a block size of less than 512 bytes, the library will
provide it by storing your (less than 512-byte) blocks in 512-byte
blocks because the metadata can't address blocks smaller than 512
bytes. I guess I could just remove the text because the behavior is
correct either way, but I was trying to make it clear why you might
not get as much usable space as you expect when you use small block
sizes. Hmm. Maybe I should word-smith that text a bit better?

> Also consider adding some libpmem-specific error codes, especially for
> things like the EINAL returned for a corrupted pool.

The age-old decision for library writers is whether to use errno,
allowing things like perror() to work, or to make up new codes. I
went the errno way. But for libpmemobj, I ended up adding another
interface to fetch more detail. Maybe I should add that for the other
modes as well. I'll ponder this...

> Why is the minimum block pool size 1GB?

We just picked a nice round number. The layout works at smaller
sizes. We were just chatting about this in the team and I think we're
going to update the spec to 16MB as a min size.

> size_t pmemblk_nblock(PMEMblk *pbp);
>
> Again, from the docs:
> The pmemblk_nblock() function returns the usable space in the
> block memory pool, expressed as the number of blocks available.
> pbp must be a block memory pool as returned by pmemblk_map().
>
> It may be helpful to clarify that this is not the amount of free space.

My terseness got the better of me here I think. pmemblk_usable()?
pmemblk_usable_blocks()?

I chose "nblock" because it is so common in C to do for loops from
zero to < nelement in an array.

> For pmemlog, I had anticipated a ring buffer. Do you expect the users
> of the pmemlog api to create multiple log files, and switch between them
> as they fill up, with a background process pushing data out from full
> log files? Had you considered a ring buffer for this?

I did think about this, and that might be a cool library to have in
the future. Although the more general pmemobj library will allow you
to create a transactional ring buffer so maybe we just tell people to
use that. We went the way we did because we already did a
proof-of-concept with the Redis database and this is what it required.
Eventually we plan on pushing the redis example to github as well, by
the way.

> In general, it would be helpful if you mentioned the intended design for
> an application making use of these apis.

Agreed. We did write small examples (in the "examples" directory) but
I still get your point. Frankly, the blk and log stuff was just to
get us started, figuring out how to test this stuff, implement common
internal routines, etc. It is 2% of the NVML project. The
interesting 98% of the project is the transactions and I'm planning
much more detailed documentation including a tutorial on how to use
the API once I get far enough on the API to do that.

> Notably missing is a persistent memory allocator. I'm guessing this will be
> tied to the transaction api. Is that right?

Correct.

> log levels -- give them names/#defines

I actually argued with other team members on this. I like the numbers
because they match the man pages and are small and clean. Eventually
I think I'm going to get shouted down on this one :-)

> #define PROCMAXLEN 2048 /* maximum expected line length in /proc files */
>
> How did you deduce that?

Just by picking a number that seems clearly large enough. Keep in
mind we're parsing files like /proc/self/smaps so if the format
changes in a way that invalidates that constant, the code won't work
anyway. Larger lines are possible in these /proc files, but not in a
way that breaks us. We're concerned about fields that appear in the
first 2048 characters (likely far less) so if the line is really long
because it contains a really long pathname on the end, we'll simply
process the rest of the line the next time around the loop and ignore
it because it isn't in the format we're scanning for.

All that said, do you recommend a different max len for this?

> * util_map_hint -- (internal) use /proc to determine a hint address for mmap()
> * - greater or equal 1TB,
> why?

We may relax the 1TB part of this hint in the future. The basic idea
is the maximize the probability that everything lines up nicely for
large pages. So 1GB would be fine. When I told the guys to start at
1TB, I was thinking it was also more likely you'd get the same
addresses each run because that area tends to be wide open. But
actually it might be better if the address is not the same each time
for security.

> return (*Func_is_pmem)(addr, len);
> no need for parens... return Func_is_pmem(addr, len);
> Unless that's just your style? Maybe you wanted to call attention to
> the fact that it's a function pointer?

You youngsters don't remember when the * was required by C :-) I was
trying to call attention to the fact it's a pointer, but my convention
of using a capital letter for globals already does that just fine.
Maybe I should get with the 90's and stop typing that *.

> #define PMEMTRN_MIN_POOL ((size_t)(1024 * 1024 * 2)) /* min pool size: 2MB */
>
> comment tells what but not why... why 2MB?

Placeholder. I'm sure pmemobj (which is the new name for pmemtrn)
will have a min size. Don't know what it is yet or why.

> log.h:
> #define LOG_FORMAT_DATA_ALIGN 4096
>
> I think this wants to be pagesize? If so, a configure script would be
> able to generate a config.h with the appropriate value for the platform.

Actually I meant this value to be the definition of the on-media
format, regardless of page size. Of course it matches up with x86
page size to make it more likely that we can mprotect() the header,
but that's for convenience, not for correctness. If you run the
library on some machine with a different native page size, you still
want 4096 here. (We do call sysconf() to get the page size in the
library for places where we need it, but this isn't one them.)

[Jeff Moyer]

Hi, Andy,

Andy Rudoff <an...@rudoff.com> writes:

> Great comments, Jeff. I appreciate it! Some responses in-line below...
>
> -andy
>
>> I hesitate to start looking at or thinking about the transaction apis
>> without first catching up with you. I see that the github repo is
>> pretty recently updated, so that's promising. Is there any substantial
>> amount of work that you've done privately that hasn't yet gone into
>> github? I'm trying to figure out where I can help out, and, more
>> importantly, I don't want to waste time on things you've already done.
>
> The team is currently working on prototyping the pmemobj APIs and I'll
> be posting details on that soon. But don't let that discourage you
> from working on something. If you have an area you'd like to work on,
> I suggest creating an issue in the issues database so we know you're
> working on it. Hmm. Maybe I should do that for the stuff I'm working
> on :-)

That sounds like a plan.

>> In general, I'm not sure why the map functions do not support all of
>> the arguments that mmap does. This makes things like extending the
>> file cumbersome.
>
> pmem files are like file systems. They contain data structures that
> live across crashes and need to be kept consistent. So you can't just
> grow the file and expect the right thing to happen any more than you
> can grow a partition and expect the file system to just work --
> there's usually an administrative "grow" command if growing is
> supported. That's the same strategy we're using here -- when growing

Consider just the libpmem part. I don't see a reason why someone
couldn't or wouldn't want to increase the size of such a file. For the
rest of the formats, I agree with what you're saying. I also see that
an application could use the pmem_persist/flush/fence/drain on a file it
mmap'd itself, so you can ignore this comment.

> Let me think about this one. Today when you create a file system, you
> tell the system how much storage you're willing to commit to that file
> system (by sizing the partition) and you get the usable amount of data
> that the file system gives you after it creates its metadata. We're
> following the same model. I've never seen a system where you say "I
> want 500GB of usable space, create whatever size partition you need to
> so ext4 gives me that." But it is an interesting idea. Let me ponder
> it a bit...

I guess I had been thinking of it from the POV of an application writer
who knows how much data she wants to store. Perhaps that's not a
typical use case.

> The answer is pool-specific. Some pool types, like pmemblk, are meant
> to be portable across different machine types. So the (soon to be
> published) specification for it states the exact format of all
> metadata, namely that it must be little endian, specific field sizes,
> etc. This is how most file systems work -- the metadata is made
> portable, but the user data can be anything, either non-portable or
> portable, depending on what the app writer decides to do. What would
> storing the endianness in the header do in this case? Pmemblk is
> always LE.

I hadn't thought that one through, clearly. Storing endianness in the
header wouldn't do any good unless you actually exposed that to the
application. I think your answer makes sense.

>>> From the pmemblk_map documentation:
>> There are no restrictions on the block size bsize, however libp-
>> mem will silently round up the given size to PMEMBLK_MIN_BLK, as
>> defined in <libpmem.h>.
>>
>> That sounds dangerous, especially given your read and write routines don't
>> take a length argument. I would *strongly* urge you to fail invocations
>> that do not have proper alignment.
>
> Nothing to do with alignment. The text you quoted is just saying that
> if you ask for a block size of less than 512 bytes, the library will
> provide it by storing your (less than 512-byte) blocks in 512-byte
> blocks because the metadata can't address blocks smaller than 512
> bytes. I guess I could just remove the text because the behavior is
> correct either way, but I was trying to make it clear why you might
> not get as much usable space as you expect when you use small block
> sizes. Hmm. Maybe I should word-smith that text a bit better?

Ah, I totally misinterpreted that. Adding a couple more sentences to
that paragraph would clear things up, I think.

>> Also consider adding some libpmem-specific error codes, especially for
>> things like the EINAL returned for a corrupted pool.
>
> The age-old decision for library writers is whether to use errno,
> allowing things like perror() to work, or to make up new codes. I
> went the errno way. But for libpmemobj, I ended up adding another
> interface to fetch more detail. Maybe I should add that for the other
> modes as well. I'll ponder this...

It would make sense if the application could take a different action
based on the result. Take the pmemblk_map function, for example. You
can have EINVAL returned for 2 different reasons: corrupt pool header,
or invalid pool type. I think it's worth differentiating the two,
though you could argue that the application could then call the _check
routines to determine if it is a valid pool of any type. That sounds a
bit suboptimal to me, though.

>> Why is the minimum block pool size 1GB?
>
> We just picked a nice round number. The layout works at smaller
> sizes. We were just chatting about this in the team and I think we're
> going to update the spec to 16MB as a min size.

OK. Any time I see a hard coded value without a comment, I question
it. Just add a comment for these arbitrary values and we'll call it
good. :)

>> size_t pmemblk_nblock(PMEMblk *pbp);
>>
>> Again, from the docs:
>> The pmemblk_nblock() function returns the usable space in the
>> block memory pool, expressed as the number of blocks available.
>> pbp must be a block memory pool as returned by pmemblk_map().
>>
>> It may be helpful to clarify that this is not the amount of free space.
>
> My terseness got the better of me here I think. pmemblk_usable()?
> pmemblk_usable_blocks()?
>
> I chose "nblock" because it is so common in C to do for loops from
> zero to < nelement in an array.

I don't take issue with the name, I just felt the text could use
clarification.

>> For pmemlog, I had anticipated a ring buffer. Do you expect the users
>> of the pmemlog api to create multiple log files, and switch between them
>> as they fill up, with a background process pushing data out from full
>> log files? Had you considered a ring buffer for this?
>
> I did think about this, and that might be a cool library to have in
> the future. Although the more general pmemobj library will allow you
> to create a transactional ring buffer so maybe we just tell people to
> use that. We went the way we did because we already did a
> proof-of-concept with the Redis database and this is what it required.
> Eventually we plan on pushing the redis example to github as well, by
> the way.

OK, I guess that sort of answers the "how do you expect one to use this
api" question.

>> In general, it would be helpful if you mentioned the intended design for
>> an application making use of these apis.
>
> Agreed. We did write small examples (in the "examples" directory) but
> I still get your point. Frankly, the blk and log stuff was just to

I read those examples. They are very basic. :)

> get us started, figuring out how to test this stuff, implement common
> internal routines, etc. It is 2% of the NVML project. The
> interesting 98% of the project is the transactions and I'm planning
> much more detailed documentation including a tutorial on how to use
> the API once I get far enough on the API to do that.

Sounds good.

>> #define PROCMAXLEN 2048 /* maximum expected line length in /proc files */
>>
>> How did you deduce that?

[snip]

> All that said, do you recommend a different max len for this?

No, I just wanted to know where it came from.

>> * util_map_hint -- (internal) use /proc to determine a hint address for mmap()
>> * - greater or equal 1TB,
>> why?
>
> We may relax the 1TB part of this hint in the future. The basic idea
> is the maximize the probability that everything lines up nicely for
> large pages. So 1GB would be fine. When I told the guys to start at
> 1TB, I was thinking it was also more likely you'd get the same
> addresses each run because that area tends to be wide open. But
> actually it might be better if the address is not the same each time
> for security.
>
>> return (*Func_is_pmem)(addr, len);
>> no need for parens... return Func_is_pmem(addr, len);
>> Unless that's just your style? Maybe you wanted to call attention to
>> the fact that it's a function pointer?
>
> You youngsters don't remember when the * was required by C :-) I was

You can't simultaneously call me a youngster and also use --std=gnu99. :)

Cheers,
Jeff