Page MenuHomeFreeBSD

Revert VNET change and expand VNET structure.
Needs ReviewPublic

Authored by bz on Wed, Jan 8, 11:46 PM.

Details

Reviewers
hselasky
kevans
Summary

Revert the change replacing vnet_state with a shutdown flag.
Expand the VNET structure with the new boolean flag and pad the
structure out and update the vnet magic cookie for the change.

Update libkvm to compile with a bool in the kernel struct.

Update net/if.c to make use of the old-new shutdown state.

Diff Detail

Repository
rS FreeBSD src repository
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 28557
Build 26607: arc lint + arc unit

Event Timeline

bz created this revision.Wed, Jan 8, 11:46 PM
bz added a comment.Wed, Jan 8, 11:47 PM

For @hselasky: this does not yet fix the vnet0 issue. That'll be sorted out after this.

hselasky added inline comments.Thu, Jan 9, 8:38 AM
sys/net/if.c
1084

I think ? true : false is not needed.

1371

Ditto.

1425

Ditto.

sys/net/vnet.c
356

Should there be a dummy sysuninit aswell, forcing the vnet state off DONE state?

sys/net/vnet.h
79

Is the size of bool always constant or should you use uint8_t ?

lutz_donnerhacke.de added inline comments.
sys/net/vnet.h
79

You mean for using a bit field?

uint8_t vnet_shutdown:1;

Is all the "spare" part for some alignment constraints?
If yes, can we make this explicit?

static_assert(xxx == sizeof(struct vnet), "ABI requires a fixed size of the structure");

Currently the "spare" part is fragmented and unused, can we use a simple padding instead?

uint8_t _vnet_padding[xxx];
hselasky added inline comments.Thu, Jan 9, 12:03 PM
sys/net/vnet.h
79

Yes, if only for use with same architecture, bitfields are better in my opinion.

Maybe:
u_int vnet_XXXX:1;
u_int vnet_reserved:31:

Pad it to something like a CACHE_LINE size.

The __packed keyword may cause an overhead on ARM.

Maybe better to use explicit __aligned() for all integer fields ....

bz added inline comments.Fri, Jan 10, 3:23 PM
sys/net/if.c
1084

Yes, it is not necessary but it also doesn't hurt. The result of a && operation is an int. It's just explicit boolean rather than implicit cast.

sys/net/vnet.c
356

Not sure I understand that question. If you mean a VNET_SYSUNINIT() then the answer is 'no' as the very next operation when reaching the end of SYSUINITs is to call free.

sys/net/vnet.h
79

If we want to use a bool for shutdown we should start treating it as a bool and write it as a bool; if we do a bitfield we'll do vnet_flags & VNET_FLAGS_SHUTDOWN and given we do bool in the kernel I see no reason anymore.

The size of a bool is at least big enough to hold 1 or 0. generally sizeof(bool) seems to be 1 but it could be bigger.

"spare" is a common name as a placeholder for padding; if you need to reserve something you give it a better name or a comment. It is to avoid implicit padding and the immediate need to change the size of the struct in the future for, say, other internal state.

There is a problem however that on 32bit this might still be true given the sizeof(void *)...

82

I think the __packed might not be needed.