Page MenuHomeFreeBSD

Revert VNET change and expand VNET structure.
AcceptedPublic

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

Details

Reviewers
hselasky
kevans
Group Reviewers
network
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 29017
Build 26989: arc lint + arc unit

Event Timeline

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

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

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

I think ? true : false is not needed.

1371

Ditto.

1427

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.Jan 9 2020, 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.Jan 10 2020, 3:23 PM
sys/net/if.c
1082

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.

Ping - is this patch still relevant?

bz added a comment.Fri, Jan 24, 4:18 PM

Ping - is this patch still relevant?

@hselasky , Yes, still relevant. Still not sure what you meant in one of the comments ("dummy sysuninit")?

I mean that vnet0 behaves like any other vnet regarding shutdown and state fields.

bz added a comment.Fri, Jan 24, 9:16 PM

I mean that vnet0 behaves like any other vnet regarding shutdown and state fields.

AS my very first comment on this review said: this does not fix the vnet0 issue; you only want that to be fixed for startup anyway; vnet0 shutdown does not happen; we just pull the reset pin as we've done for decades; otherwise any possible vent shutdown issue would also yield on a non-VIMAGE kernel and panic machines on reboot; not what you want.

bz updated this revision to Diff 67461.Wed, Jan 29, 4:59 PM

Address reviewer comments.

bz added a reviewer: network.Wed, Jan 29, 5:01 PM
bz marked 7 inline comments as done.
bz added inline comments.
sys/net/vnet.c
356

As I mentioned early in the review vnet0 will be dealt with separately.
Also you mean sysinit up there, not sysuninit, that's what had me confused all the time.

bz added a comment.Sun, Feb 16, 4:53 PM

Anyone any further comments on this? I'd love to commit it the next days.

bz added inline comments.Sun, Feb 16, 4:53 PM
UPDATING
29

Date will be updated accordingly.

hselasky accepted this revision.Sun, Feb 16, 5:51 PM
This revision is now accepted and ready to land.Sun, Feb 16, 5:51 PM
melifaro added inline comments.Sun, Feb 16, 6:36 PM
sys/net/if.c
1081

As this check is used 3? times: can it be a vnet macro like VNET_IS_SHUTDOWN(_vnet)?