Revert VNET change and expand VNET structure.
ClosedPublic
Actions

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

Details

Reviewers

• hselasky
kevans

Group Reviewers

network

Commits

rS358020: Partially revert VNET change and expand VNET structure.

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 - subversion

Lint

No Lint Coverage

Unit

No Test Coverage

Build Status

Buildable 29442
Build 27319: arc lint + arc unit

Event Timeline

bz created this revision.Jan 8 2020, 11:46 PM

Herald added subscribers: ae, imp. · View Herald TranscriptJan 8 2020, 11:46 PM

Harbormaster completed remote builds in B28557: Diff 66523.Jan 8 2020, 11:46 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
1089	I think ? true : false is not needed.
1377	Ditto.
1432	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 ?

donner added a subscriber: donner.Jan 9 2020, 11:59 AM

donner 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
1089	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?

In D23097#511548, @hselasky wrote:

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.

In D23097#511996, @hselasky wrote:

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.

Address reviewer comments.

Harbormaster completed remote builds in B29017: Diff 67461.Jan 29 2020, 4:59 PM

bz added a reviewer: network.Jan 29 2020, 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.