Em Wed, Apr 03, 2019 at 12:52:01PM -0700, Andrii Nakryiko escreveu:
> 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).
Perhaps we can just disable moving bitfields around for now, then
revisit this, so that we leave, again, for now, just the moves that do
not produce inconsistencies. I'll look into that.
- Arnaldo
> >
> > [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
--
- Arnaldo
