TCP and checksum offloading for Hyper-V

[whu]

Summary

Netvsc TCP and checksum offloading for Hyper-V. Also fixed some bugs
in the netvsc driver.

This is for the head branch. We are targeting this for 10.2 release once
this is in the head.

Test Plan

Tested by Microsoft OSTC.

[whu]

• Incorporate internal review comments

[whu]

• Fix white space issue left from previous change

[royger]

I cannot really comment on the specific implementation since I don't know anything about the HyperV net protocol. I have however found quite a lot of coding style errors, and a couple of minor comments/improvements.

sys/dev/hyperv/netvsc/hv_net_vsc.c

146 ↗

(On Diff #5469)

Parentheses around return value.

151 ↗

(On Diff #5469)

No need for braces in single line statements.

156 ↗

(On Diff #5469)

Parentheses around return value.

311 ↗

(On Diff #5469)

2 casts? Why not use uint64_t directly?

335 ↗

(On Diff #5469)

I see that you use M_DEVBUF in the driver, could you create a new tag to be used by the net driver? M_NETVSC sounds like a suitable name.

591 ↗

(On Diff #5469)

Adding braces in the for case would make this code easier to read (not that they are really needed).

603 ↗

(On Diff #5469)

Can you use device_printf here?

sys/dev/hyperv/netvsc/hv_net_vsc.h

869 ↗

(On Diff #5469)

Is this 4096 because it's limited by PAGE_SIZE, or it is not related?

912 ↗

(On Diff #5469)

uint8_t?

1003 ↗

(On Diff #5469)

Please indent using 4 spaces (here and below).

1005 ↗

(On Diff #5469)

Following the style used in this file I don't think there should be a line break here.

sys/dev/hyperv/netvsc/hv_netvsc_drv_freebsd.c

214 ↗

(On Diff #5469)

IMHO I would set ret_val = TRANSPORT_TYPE_NOT_IP here so the flow is clearer.

sys/dev/hyperv/netvsc/hv_rndis.h

652 ↗

(On Diff #5469)

Missing space.

1057 ↗

(On Diff #5469)

Coding style, and why are those declared as extern?

sys/dev/hyperv/netvsc/hv_rndis_filter.c

84 ↗

(On Diff #5469)

This is not the FreeBSD coding style, please fix it. Also why are you using extern here?

108 ↗

(On Diff #5469)

Extern?

115 ↗

(On Diff #5469)

Missing parentheses.

342 ↗

(On Diff #5469)

Missing parentheses around return value, and no minus sign please.

374 ↗

(On Diff #5469)

Missing space between ) and {, here and in several other places.

378 ↗

(On Diff #5469)

What does this value mean? Can't you use a proper error value (if it's indeed an error)?

873 ↗

(On Diff #5469)

Missing space between ) and {.

[whu]

Thanks for the review. I have addressed most of them. Only have a question regarding to creating a new tag for M_DEVBUF.

sys/dev/hyperv/netvsc/hv_net_vsc.c

335 ↗

(On Diff #5469)

Hmm.... why do we need to create a new one while the existing M_DEVBUF could serve the purpose?

sys/dev/hyperv/netvsc/hv_net_vsc.h

869 ↗

(On Diff #5469)

This was inherited from its Windows counterpart. I believe it is somewhat related to PAGE_SIZE.

[royger]

sys/dev/hyperv/netvsc/hv_net_vsc.c

335 ↗

(On Diff #5469)

So that when you use vmstat -m it's not listed as a generic "devbuf" allocation. It's quite helpful when debugging memory leaks or track memory usage of each driver.

sys/dev/hyperv/netvsc/hv_net_vsc.h

869 ↗

(On Diff #5469)

IMHO, if this is related to page size I would rather use PAGE_SIZE directly.

[whu]

• Incorporate review comments from royger

[royger]

I've still found quite a lot of coding style errors, please try to check those before sending patches for review or else they just slow down the review process.

sys/dev/hyperv/netvsc/hv_net_vsc.c

68 ↗

(On Diff #5736)

This should be aligned using 4 spaces after a line break.

131 ↗

(On Diff #5736)

extern in function definition???

150 ↗

(On Diff #5736)

This is defined inside of the Xen headers, you might want to use atomic_testandset_long in the machine/atomic.h header instead.

180 ↗

(On Diff #5736)

contigmalloc assumes M_WAITOK unless M_NOWAIT is specified, so you can drop the check against NULL.

291 ↗

(On Diff #5736)

Stray change, or is it an alignment fix? Sorry but phabric is quite crappy with tabs vs spaces.

334 ↗

(On Diff #5736)

net_dev->bitsmap_words = howmany(net_dev->send_section_count, BITS_PER_LONG);

495 ↗

(On Diff #5736)

After a line break align using 4 spaces.

578 ↗

(On Diff #5736)

If this always has to be the number of items in the static array initialized above I would rather use:

int protocol_number = nitems(protocol_list);

597 ↗

(On Diff #5736)

device_printf?

773 ↗

(On Diff #5736)

Align using 4 spaces after a line break.

798 ↗

(On Diff #5736)

Again this is defined in Xen headers. You should be able to find a suitable equivalent in machine/atomic.h. I plan to remove those Xen specific bitops and replace them with the FreeBSD native ones in Xen code.

872 ↗

(On Diff #5736)

This is not FreeBSD coding style, please fix it.

887 ↗

(On Diff #5736)

device_printf

896 ↗

(On Diff #5736)

device_printf

904 ↗

(On Diff #5736)

device_printf

943 ↗

(On Diff #5736)

4 spaces after line break, and please place both in the same line.

1008 ↗

(On Diff #5736)

device_printf

1024 ↗

(On Diff #5736)

device_printf

1030 ↗

(On Diff #5736)

IMHO you should consider removing this unbound loop, using this kind of constructions can lead to infinite loops in kernel code. Can't you use ret as a condition here and avoid the breaks in the inner code?

sys/dev/hyperv/netvsc/hv_net_vsc.h

943 ↗

(On Diff #5736)

IMHO I think you should be able to use LONG_BIT from sys/limits.h instead of rolling your own. Or in any case this should be:

#ifdef  __LP64__
#define BITS_PER_LONG 64
#else
#define BITS_PER_LONG 32
#endif

Although I would recommend to simply set:

#define BITS_PER_LONG LONG_BIT

1007–1009 ↗

(On Diff #5736)

there's no reason to add a line break between tid and status. Also, could you remove the "extern" from function prototypes? IMHO unless I'm wrong it simply makes no sense, function declarations are always extern in C.

sys/dev/hyperv/netvsc/hv_netvsc_drv_freebsd.c

429 ↗

(On Diff #5736)

Don't add this alignment here, the reset of the local variables are aligned using a space.

469 ↗

(On Diff #5736)

device_printf

495 ↗

(On Diff #5736)

device_printf

674 ↗

(On Diff #5736)

atop(hv_get_phys_addr(rndis_mesg))

676 ↗

(On Diff #5736)

Use PAGE_MASK instead of PAGE_SIZE - 1

850 ↗

(On Diff #5736)

This is not FreeBSD coding style.

890 ↗

(On Diff #5736)

device_printf and no exclamation marks.

sys/dev/hyperv/netvsc/hv_rndis.h

1050 ↗

(On Diff #5736)

4 spaces after line break.

1053 ↗

(On Diff #5736)

4 spaces after line break.

sys/dev/hyperv/netvsc/hv_rndis_filter.c

74 ↗

(On Diff #5736)

4 spaces after line break.

227 ↗

(On Diff #5736)

4 spaces after line break.

311 ↗

(On Diff #5736)

4 spaces after line break.

355 ↗

(On Diff #5736)

device_printf

361 ↗

(On Diff #5736)

device_printf

367 ↗

(On Diff #5736)

device_printf

393 ↗

(On Diff #5736)

switch (indicate->status) {
case RNDIS_STATUS_MEDIA_CONNECT:
...
case RNDIS_STATUS_MEDIA_DISCONNECT:
...
default:
...

398 ↗

(On Diff #5736)

Should you add a device_printf here indicating that an unknown change has been received, and thus will be ignored?

426 ↗

(On Diff #5736)

device_printf, and please write complete error messages (total, length). Those are supposed to be understood by users.

871 ↗

(On Diff #5736)

device_printf

sys/dev/hyperv/netvsc/hv_rndis_filter.h

102 ↗

(On Diff #5736)

Why all those externs in function declarations?

[whu]

Sorry it took a while to get this done. I was out of office due to family issues.

sys/dev/hyperv/netvsc/hv_net_vsc.c

798 ↗

(On Diff #5736)

I don't find similar one in atomic.h using 'btcl' instruction.

1030 ↗

(On Diff #5736)

In the cases of either (ret == 0 && bytes_rxed > 0) or (ret == ENOBUFS), we still want to keep reading the ring buffer until there is nothing left. The only case to break out of the while loop is (ret ==0 && bytes_rxed == 0).

If you don't mind, I would like to maintain its current code logic to make it in consistence to its Windows and Linux counterparts.

[whu]

• More review comments from royger

[royger]

In D2517#54874, @whu wrote:

• More review comments from royger

I'm happy with the code now. I've just added a couple of comments, please make sure to read the ones about the atomic_ and synch_ ops, since they may affect your code (and sorry for giving a wrong advice in the first place).

Once that's done, you can commit it.

sys/dev/hyperv/netvsc/hv_net_vsc.c

148 ↗

(On Diff #6262)

Is "bitsmap" shared between the VM and another entity on the system (another VM or the hypervisor)?

If that's the case please ignore my previous comment and keep using the synch_* functions, or else it won't work properly when used on a kernel without SMP support.

789 ↗

(On Diff #6262)

Right, this is missing from the native FreeBSD atomic ops. I've also realized that on UP kernels (kernels compiled without SMP) the native FreeBSD atomic ops drop the "lock" prefix, which means they cannot be safely used inside of a VM if the memory is shared between another VM or the hypervisor.

I guess it's fine to use the synch_* ones for now. I will try to come up with a patch to properly remove them when I have some time...

1023 ↗

(On Diff #6262)

As you wish, this was just a suggestion. At the end of day you are the one that's going to maintain the code :).

[whu]

Thanks Roger for the review!

sys/dev/hyperv/netvsc/hv_net_vsc.c

148 ↗

(On Diff #6262)

Thanks so much Roger! I didn't realize it. You are absolutely right. It just uses MPLOCK which could cause problem in UP VMs. I will change it back to use synch_* as it is likely this bitmap is being shared by hypervisor and VMs.