Grant-Table User-Space Device

[akshay1994.leo_gmail.com]

Summary

A grant-table user-space device will allow user-space applications to map and share grants (Xen way to share memory) among Xen domains.

Test Plan

Tested using a custom test

freebsd_interactive_test.c6 KBDownload

and xen qdisk backend

xen_freebsd.patch14 KBDownload

.

[pfg]

The Xen code may have it's own style guide but it might be good to check the style(9) manpage and perhaps the indent(1) example in /usr/share/examples.

sys/dev/xen/gntdev/gntdev.c

6

These two lines should be moved after the license. The license should be strictly as suggested here:

https://www.freebsd.org/doc/en/articles/committers-guide/pref-license.html

37

Instead of this, you can just add $FreeBSD$ in the comment, after the license.

[imp]

sys/dev/xen/gntdev/gntdev.c

6

To be clear, the lines before the copyright line are what should be moved, not the copyright line.

37

For .c files, this is the correct way. For .h files, you are correct.

[pfg]

sys/dev/xen/gntdev/gntdev.c

37

Hmm.. then I guess I have screwed up more than once :(.

[royger]

Thanks, the code looks good, but it's a complicated device and there are no comments almost anywhere describing what's going on. Please add comments to the major functions (as described in style(9).

I would appreciate if you could add a comment at the top of the file describing how a user-space client is expected to use the device. The order of the operations and what's done in each one of them, for example if the client first has to issue an ioctl and then a mmap, and which parameters should be used, you can use a simple example in order to clarify all this.

sys/dev/xen/gntdev/gntdev.c

159

The bracket should be on the same line as the iterator.

161

Same here, this should be on the same line as the if (there are a bunch of those, so I'm going to stop commenting on each one of them).

197

Would it be better to use a red-black tree in order to store the list of grants from a performance point of view? I expect that the list is going to be mostly accessed in order to search for granted regions rather than adding or removing them.

There's an example of a red-black tree usage in sys/vm/vm_phys.c, search for the RB_* macros :).

234

So you do the cleanup of expired grefs when new ones are mapped, shouldn't it be better to defer the cleanup to some background task, so that it doesn't interfere with the allocation of new grefs? Take a look at the TASKQUEUE(9) man page, it allows to schedule asynchronous task execution.

243

gref cannot be NULL, because you are suing M_WAITOK, so you can remove this check (or turn it into a KASSERT if you are paranoid :))

289

Same here, you should probably look into making gref_list_dtor a task that you can schedule in the background, because AFAICT there's no obvious reason that it should be explicitly done at this point.

296

You should not declare variables here, instead do it at the top of the function (didn't you get a complain from the compiler here?)

299

No need for the brackets here.

325

gref == NULL (gref is not a boolean, so you should not use !).

428

malloc with M_WAITOK cannot fail (there are a couple more below which I'm not going to comment on).

471

Why do you need to set handle to -1 and host_addr to 0, aren't you just unmapping the grants?

635

It seems like indentation is off here, but maybe it's just phabricator. Could you make sure you indent the lines that you have to break (because they are too long) using 4 spaces.

675

Don't mix declarations with code, this should be declared at the beginning of the function.

764

All the cases are indented one level more than required. "switch" and "case" should be at the same indent level.

835

You should create two separate helpers here, one for the gnt map and one for the gnt share cases, in order to help the readability of the code.

927

This comments (here and below) don't follow the coding style, check style(9).

989

If the function doesn't have any local variables an empty newline should be left at the top (here and below).

sys/xen/gntdev.h

41

Hm, why did you add this C++ compat thing? None of the FreeBSD headers for Xen user-space devices have it AFAIK.

[akshay1994.leo_gmail.com]

Style Corrections

[akshay1994.leo_gmail.com]

I've added comments to all parts of the code, and tried to conform with the style guidelines everywhere.
I'll add instructions on how to use the device in the header file in an hour.

I haven't added TASKQUEUE or RB_TREE yet, because I thought it will be better that this version go in first, and then incrementally add these things. Otherwise it will make debugging a bit difficult, especially with Async tasks, the code is anyway too complex. If this version works perfectly, I'll add these performance improvements later on. There are other things we can (and have to) improve on too, one of which is file offset allocation, which is on my mind. Also, notify is yet to be implemented.

:)

sys/dev/xen/gntdev/gntdev.c

197

It might be an overkill, given that we access the list only twice or thrice for every allocated grant (mmap, maybe notify, and dealloc). Linux uses a list.

234

I guess it should be rare that some domain keeps an allocated grant mapped, even when access from this side has ended. This function is always called when any grant is deallocated, but just to kill phantom grants still alive, I call it here.
Linux also calls it, but I guess their reasons are different, they want to ensure max number of free grants are available before the allocation happens.

289

Since this is an error, mostly only free() calls will happen here.

471

This function might be called in case the Map Hyper-call fails partially (say half of the grants are mapped and the rest are not) (line 924). In that case, I enumerate over the successful maps, and un-map them. Since the user can retry the mmap in such a case, to redo the hyper-call, I update the internal data-structures to represent the fact that none of the grants are mapped anymore. After this, the state of the internal data structures is same as it was before the mmap had happened.

sys/xen/gntdev.h

41

I've removed it. It's the default template for C header files on my system. It's not required here, as we just have structure definitions, and even if there were any declarations, I guess the kernel doesn't have any C++ code that would require this.

[pfg]

In D7293#153526, @akshay1994.leo_gmail.com wrote:

...

I haven't added TASKQUEUE or RB_TREE yet, because I thought it will be better that this version go in first, and then incrementally add these things. Otherwise it will make debugging a bit difficult, especially with Async tasks, the code is anyway too complex.

The taskqueue(9) and tree(3) stuff has been tested very well as it is used in the rest of the system. There is no good reason not to use it from the start.

[akshay1994.leo_gmail.com]

The taskqueue(9) and tree(3) stuff has been tested very well as it is used in the rest of the system. There is no good reason not to use it from the start.

Mhm. Noted. I'll add them now. :)

[akshay1994.leo_gmail.com]

Added Async cleanup using Taskqueue and RedBlack Tree for internal data structures.
Please review :)

[royger]

Just some initial thoughts, I will try to do a more in-deep review tomorrow.

sys/dev/xen/gntdev/gntdev.c

161

With the addition of the RB tree now the structure requires two different handlers, one for the RB tree and another one for the TAILQ, which is rather unfortunate (also taking into account that there might be hundreds of those on a busy Dom0.

Could you please benchmark if the usage of the RB tree makes any difference from a performance point of view?

In order to test it you should create a NULL memdisk (all reads will return 0, writes are ignored) using:

# mdconfig -a -t null -s 10g

Attach it to a guest using the Qdisk backend and then run fio (or any other disk benchmark) against the raw block device. You should test the pre-RB and the RB versions of the gnttab device and decide if the addition of the RB tree is worth it or not.

187

Since this now runs in a taskqueue you should use a STAILQ list, and only hold the mutex in order to pick the first element from the queue and remove it. The unmap and cleanup should be done without holding the mutex in order to prevent contention. If the element can not be removed you will have to put it into a local queue that will be appended to the global one once processing has finished. You will have to do it that way in order to prevent iterating forever over grants that cannot be unmapped.

[akshay1994.leo_gmail.com]

sys/dev/xen/gntdev/gntdev.c

161

Consistently better performance using RB tree. I guess we should keep it :)

P.S. - And even better performance using (RB + Async) :) :)

with_RB4 KBDownload

with_RB_run24 KBDownload

with_RB_run34 KBDownload

without_RB_run34 KBDownload

without_RB_run24 KBDownload

without_RB4 KBDownload

fio_test.sh839 BDownload

with_RB_and_Async5 KBDownload

187

Done, captain!

[akshay1994.leo_gmail.com]

Add notify function UNMAP_NOTIFY_CLEAR_BYTE.

[royger]

sys/dev/xen/gntdev/gntdev.c

161

Great, it looks marginally better.

FWIW, in FreeBSD (and other projects), when you give performance figures you usually have to use a table of some sort, with the relevant data, and the stddev. Without this data results could be considered useless. If you want to give this a try, there's a very good cli tool in FreeBSD called 'ministat(1)', that will give you a nice graph and will tell you whether there's a performance benefit or it's just an statistic illusion. IMHO it's not necessary in this case, but I would recommend that you give it a try if you want/can.

161

In order to reduce the size of this struct, you should put gref_list_next and gref_tree_next inside of a union, since AFAICT, you will never have a gref added to both the cleanup tree and a RB tree (same applies to gntdev_gmap).

And the struct definitions should all be at the top of the file. Having functions interleaved with struct definitions makes the code harder to read IMHO (I might be convinced otherwise if you think it's better the other way around).

936

I don't see any reason for this ioctl to exists, it's not implemented and will never be. Should be removed from the header too.

[akshay1994.leo_gmail.com]

Added UNMAP_NOTIFY_SEND_EVENT code, factored out notify function, used a union to save space, and added usage instructions in the device header file.

[akshay1994.leo_gmail.com]

• Header file updated with usage instructions.
• set_max_grants ioctl removed.
• Refactored notify and event-channel notification code.

sys/dev/xen/gntdev/gntdev.c

161

Noted. I'll keep it in mind from next time onwards.

161

I've shifted both iterator structures inside a union.

I'm a little inclined towards keeping the struct definitions in the middle because the code is divided into segments, as follows:

1. Global Device Data-Structures
2. Grant Allocation Data-Structures and Methods
3. Grant Mapping Data-Structures and Methods
4. Grant Mapping Pager
5. Grant Device Methods
6. Character Device Methods (ioctl/mmap...)
7. Private Device Attachment Functions

The DS defined in (2) has no use in any of the other parts of the code, and the same is true for the DS defined in (3). Most of their use is inside their respective sections, and thus I defined them at the beginning of their respective sections.

936

Removed. :)

[royger]

LGTM, just one minor comment.

sys/dev/xen/gntdev/gntdev.c

765

Shouldn't there be some check to make sure you don't write past the mapped region? (Maybe it's somewhere else, but I'm not able to find it).

829

s/(PAGE_SIZE - 1)/PAGE_MASK

[akshay1994.leo_gmail.com]

sys/dev/xen/gntdev/gntdev.c

765

Yes. The check is already present.
The check is done when choosing the gmap/gref to make the notify structure into. Line 840-841 for gmap, and function call to gntdev_find_grefs(.., index, ..) at line 829 for gref.

[royger]

sys/dev/xen/gntdev/gntdev.c

120

Now that I look again into this, isn't this a little bit too simplistic? At some point priv_user->file_offset is going to overflow, returning into 0, and then we risk overlapping with existing regions. IMHO, you should use something like a region manager (rangeset), that can keep track of start and sizes of the allocated regions, it's going to add a little bit more of logic, but at least we will be on the safe side.

[akshay1994.leo_gmail.com]

sys/dev/xen/gntdev/gntdev.c

120

Aye Aye! This was an improvement I left on for something later. (Linux uses similar simplistic logic for alloc, but perhaps something more complicated for map).
Since, we now have time, I'll look into the rangeset. :)

[akshay1994.leo_gmail.com]

Added rangeset for file-offset allocation. File offsets can now be reused. :)

Please note that this differential update hasn't been tested. I do not have access to the testing machine anymore.