On Wed, Apr 3, 2019 at 6:45 AM Arnaldo Carvalho de Melo
<arnaldo.melo@xxxxxxxxx> wrote:
>
> Em Wed, Apr 03, 2019 at 12:01:48AM -0700, andrii.nakryiko@xxxxxxxxx escreveu:
> > From: Andrii Nakryiko <andriin@xxxxxx>
> >
> > Current logic in some cases will move bitfield member and then will
> > shift all the following members by the size of underlying base type of
> > bitfield, which is wrong, as bitfield's size doesn't correspond to the
> > size of underlying base type.
> >
> > Without this fix, reorganizing struct net_device would emit BRAING
> > FARTs. Now it doesn't.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@xxxxxx>
> > ---
> > dwarves_reorganize.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/dwarves_reorganize.c b/dwarves_reorganize.c
> > index 9b3aea2..e48d8d5 100644
> > --- a/dwarves_reorganize.c
> > +++ b/dwarves_reorganize.c
> > @@ -321,7 +321,7 @@ static void class__move_member(struct class *class, struct class_member *dest,
> > }
> > } else if (from_was_last) {
> > class->type.size -= from_size + class->padding;
> > - } else {
> > + } else if (from->bitfield_size == 0) {
> > /*
> > * See if we are adding a new hole that is bigger than
> > * sizeof(long), this may have problems with explicit alignment
>
> After applying this patch I still get a BFA with a vmlinux for x86_64:
Yeah, I've been using older vmlinux and its net_device didn't hit this
case. I've recompiled latest vmlinux an now see BFAs as well.
This happens because at some step there is a bitfield that is moved
from the end of a struct into somewhere in the middle, causing some
padding adjustments to be done using from_size (which is incorrect for
bitfield and shouldn't be used for adjustments). Slightly better way
to prevent that would be to add if (from->bitfield_size == 0) {}
around this whole if/else if/else statement. But even with that, there
are other cases where bitfields are mishandled. Ultimately,
correctness and BFAs depend on all the byte offsets and bit offsets
adjustments this algorithm does, and in some cases it doesn't handle
bitfields correctly.
As you said, it will take some time to fix/rewrite this whole
algorithm. You might as well discard this patch, because it helps only
in some cases (and causes some of the padding to not be adjusted).
>
> [acme@quaco pahole]$ pahole -F btf -C net_device --reorganize vmlinux | tail
> struct lock_class_key * qdisc_running_key; /* 2200 8 */
>
> /* size: 2208, cachelines: 35, members: 135 */
> /* sum members: 2194, holes: 3, sum holes: 5 */
> /* sum bitfield members: 25 bits, bit holes: 4, sum bit holes: 303 bits */
> /* paddings: 5, sum paddings: 23 */
> /* last cacheline: 32 bytes */
>
> /* BRAIN FART ALERT! 2208 bytes != 2194 (member bytes) + 25 (member bits) + 5 (byte holes) + 303 (bit holes), diff = -256 bits */
> }; /* saved 96 bytes and 1 cacheline! */
> [acme@quaco pahole]$
>
> [acme@quaco pahole]$ pahole --show_reorg_steps -F dwarf -C net_device --reorganize vmlinux | egrep '^/|BRAIN'
> /* bitfield types were fixed */
> /* Demoting bitfield ('wol_enabled') from 'unsigned int' to 'unsigned char' */
> /* Moving 'min_header_len' from after 'hard_header_len' to after 'irq' */
> /* Moving 'hard_header_len' from after 'type' to after 'min_header_len' */
> /* Moving 'uc_promisc' from after 'name_assign_type' to after 'min_header_len' */
> /* Moving 'dev_port' from after 'dev_id' to after 'type' */
> /* Moving 'dev_id' from after 'neigh_priv_len' to after 'dev_port' */
> /* Moving 'name_assign_type' from after 'addr_list_lock' to after 'neigh_priv_len' */
> /* Moving bitfield('wol_enabled' ... 'wol_enabled') from after 'proto_down' to after 'name_assign_type' */
> /* BRAIN FART ALERT! 2240 bytes != 2194 (member bytes) + 25 (member bits) + 40 (byte holes) + 31 (bit holes), diff = -524288 bits */
> /* Moving 'needs_free_netdev' from after 'rtnl_link_state' to after 'wol_enabled' */
> /* BRAIN FART ALERT! 2248 bytes != 2194 (member bytes) + 25 (member bits) + 43 (byte holes) + 263 (bit holes), diff = -256 bits */
> /* Moving 'gso_max_segs' from after 'gso_max_size' to after 'needs_free_netdev' */
> /* BRAIN FART ALERT! 2248 bytes != 2194 (member bytes) + 25 (member bits) + 43 (byte holes) + 263 (bit holes), diff = -256 bits */
> /* Moving 'proto_down' from after 'qdisc_running_key' to after 'needs_free_netdev' */
> /* BRAIN FART ALERT! 2240 bytes != 2194 (member bytes) + 25 (member bits) + 42 (byte holes) + 263 (bit holes), diff = -256 bits */
> /* Moving 'watchdog_timeo' from after 'tx_global_lock' to after 'addr_list_lock' */
> /* BRAIN FART ALERT! 2232 bytes != 2194 (member bytes) + 25 (member bits) + 34 (byte holes) + 263 (bit holes), diff = -256 bits */
> /* Moving bitfield('rtnl_link_state' ... 'rtnl_link_state') from after 'dismantle' to after 'index_hlist' */
> /* BRAIN FART ALERT! 2232 bytes != 2194 (member bytes) + 25 (member bits) + 32 (byte holes) + 279 (bit holes), diff = -256 bits */
> /* Moving 'dismantle' from after 'reg_state' to after 'rtnl_link_state' */
> /* BRAIN FART ALERT! 2232 bytes != 2194 (member bytes) + 25 (member bits) + 29 (byte holes) + 303 (bit holes), diff = -256 bits */
> /* Moving bitfield('reg_state' ... 'reg_state') from after 'link_watch_list' to after 'dismantle' */
> /* BRAIN FART ALERT! 2232 bytes != 2194 (member bytes) + 25 (member bits) + 29 (byte holes) + 303 (bit holes), diff = -256 bits */
> /* Moving 'gso_max_size' from after 'rtnl_link_ops' to after 'reg_state' */
> /* BRAIN FART ALERT! 2224 bytes != 2194 (member bytes) + 25 (member bits) + 21 (byte holes) + 303 (bit holes), diff = -256 bits */
> /* Moving 'num_tc' from after 'dcbnl_ops' to after 'dismantle' */
> /* BRAIN FART ALERT! 2224 bytes != 2194 (member bytes) + 25 (member bits) + 21 (byte holes) + 303 (bit holes), diff = -256 bits */
> /* Moving 'dcbnl_ops' from after 'rtnl_link_ops' to after 'gso_max_size' */
> /* BRAIN FART ALERT! 2216 bytes != 2194 (member bytes) + 25 (member bits) + 13 (byte holes) + 303 (bit holes), diff = -256 bits */
> /* Moving 'rtnl_link_ops' from after 'sysfs_rx_queue_group' to after 'link_watch_list' */
> /* BRAIN FART ALERT! 2208 bytes != 2194 (member bytes) + 25 (member bits) + 5 (byte holes) + 303 (bit holes), diff = -256 bits */
> /* Final reorganized struct: */
> /* BRAIN FART ALERT! 2208 bytes != 2194 (member bytes) + 25 (member bits) + 5 (byte holes) + 303 (bit holes), diff = -256 bits */
> [acme@quaco pahole]$
>
> - Arnaldo
