On Wed, Apr 3, 2019 at 2:50 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Wed, Apr 3, 2019 at 2:36 PM Arnaldo Carvalho de Melo
> <arnaldo.melo@xxxxxxxxx> wrote:
> >
> > Em Wed, Apr 03, 2019 at 06:06:18PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > 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:
> > > > > > +++ 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.
> >
> > So, with the patch below we get to:
> >
> > [acme@quaco pahole]$ pahole --show_reorg_steps -F dwarf -C net_device --reorganize vmlinux | egrep '^/|BRAIN'
> > /* 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 'watchdog_timeo' from after 'tx_global_lock' to after 'addr_list_lock' */
> > /* Moving 'needs_free_netdev' from after 'rtnl_link_state' to after 'name_assign_type' */
> > /* Moving 'gso_max_segs' from after 'gso_max_size' to after 'rtnl_link_state' */
> > /* Moving 'gso_max_segs' from after 'rtnl_link_state' to after 'needs_free_netdev' */
> > /* Moving 'gso_max_size' from after 'rtnl_link_ops' to after 'rtnl_link_state' */
> > /* Moving 'prio_tc_map' from after 'tc_to_txq' to after 'index_hlist' */
> > /* Moving 'proto_down' from after 'qdisc_running_key' to after 'gso_max_segs' */
> > /* Moving 'dismantle' from after 'reg_state' to after 'proto_down' */
> > /* Final reorganized struct: */
> > [acme@quaco pahole]$ pahole --show_reorg_steps -F btf -C net_device --reorganize vmlinux | egrep '^/|BRAIN'
> > /* 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 'watchdog_timeo' from after 'tx_global_lock' to after 'addr_list_lock' */
> > /* Moving 'needs_free_netdev' from after 'rtnl_link_state' to after 'name_assign_type' */
> > /* Moving 'gso_max_segs' from after 'gso_max_size' to after 'rtnl_link_state' */
> > /* Moving 'gso_max_segs' from after 'rtnl_link_state' to after 'needs_free_netdev' */
> > /* Moving 'gso_max_size' from after 'rtnl_link_ops' to after 'rtnl_link_state' */
> > /* Moving 'prio_tc_map' from after 'tc_to_txq' to after 'index_hlist' */
> > /* Moving 'proto_down' from after 'qdisc_running_key' to after 'gso_max_segs' */
> > /* Moving 'dismantle' from after 'reg_state' to after 'proto_down' */
> > /* Final reorganized struct: */
> > [acme@quaco pahole]$ pahole --show_reorg_steps -F btf -C net_device --reorganize vmlinux | tail
> > unsigned int wol_enabled:1; /* 2216: 8 4 */
> >
> > /* size: 2224, cachelines: 35, members: 135 */
> > /* sum members: 2194, holes: 2, sum holes: 18 */
> > /* sum bitfield members: 25 bits, bit holes: 2, sum bit holes: 16 bits */
> > /* padding: 4 */
> > /* paddings: 5, sum paddings: 23 */
> > /* bit_padding: 23 bits */
> > /* last cacheline: 48 bytes */
> > }; /* saved 80 bytes and 1 cacheline! */
> > [acme@quaco pahole]$
> >
> > [acme@quaco pahole]$ pahole -F btf --reorganize -C task_struct vmlinux | grep BRAIN
> > [acme@quaco pahole]$ pahole -F btf --reorganize -C task_struct vmlinux | tail
> >
> > /* --- cacheline 103 boundary (6592 bytes) was 16 bytes ago --- */
> > struct thread_struct thread; /* 6608 4352 */
> >
> > /* size: 10960, cachelines: 172, members: 207 */
> > /* sum members: 10898, holes: 3, sum holes: 54 */
> > /* sum bitfield members: 10 bits, bit holes: 2, sum bit holes: 54 bits */
> > /* paddings: 3, sum paddings: 14 */
> > /* last cacheline: 16 bytes */
> > }; /* saved 48 bytes! */
> > [acme@quaco pahole]$ pahole -F btf --reorganize -C super_block vmlinux | grep BRAIN
> > [acme@quaco pahole]$ pahole -F btf --reorganize -C super_block vmlinux | tail
> > /* --- cacheline 21 boundary (1344 bytes) was 24 bytes ago --- */
> > struct list_head s_inodes_wb; /* 1368 16 */
> > struct list_head s_inodes; /* 1384 16 */
> > spinlock_t s_inode_wblist_lock; /* 1400 4 */
> > spinlock_t s_inode_list_lock; /* 1404 4 */
> >
> > /* size: 1408, cachelines: 22, members: 58 */
> > /* sum members: 1405, holes: 1, sum holes: 3 */
> > /* paddings: 2, sum paddings: 8 */
> > }; /* saved 128 bytes and 2 cachelines! */
> > [acme@quaco pahole]$
> >
> > [acme@quaco pahole]$ pahole -F btf --packable vmlinux | cut -f 1 | while read struct ; do pahole -F btf -C $struct --reorganize vmlinux | grep BRAIN ; done
> > /* BRAIN FART ALERT! 200 bytes != 176 (member bytes) + 0 (member bits) + 80 (byte holes) + 0 (bit holes), diff = -448 bits */
> > /* BRAIN FART ALERT! 760 bytes != 756 (member bytes) + 0 (member bits) + 4 (byte holes) + 0 (bit holes), diff = -32 bits */
> > /* BRAIN FART ALERT! 72 bytes != 28 (member bytes) + 0 (member bits) + 100 (byte holes) + 0 (bit holes), diff = -448 bits */
> > /* BRAIN FART ALERT! 40 bytes != 36 (member bytes) + 0 (member bits) + 44 (byte holes) + 0 (bit holes), diff = -352 bits */
> > /* BRAIN FART ALERT! 184 bytes != 182 (member bytes) + 0 (member bits) + 6 (byte holes) + 0 (bit holes), diff = -32 bits */
> > [acme@quaco pahole]$ pahole -F btf --packable vmlinux | wc -l
> > 503
> > [acme@quaco pahole]$
> >
> > 1% getting closer...
> >
> > [acme@quaco pahole]$ pahole -F btf --reorganize -C netns_frags vmlinux
> > struct netns_frags {
> > long int high_thresh; /* 0 8 */
> > long int low_thresh; /* 8 8 */
> > int timeout; /* 16 4 */
> > int max_dist; /* 20 4 */
> > struct inet_frags * f; /* 24 8 */
> > atomic_long_t mem; /* 32 8 */
> >
> > /* XXX 24 bytes hole, try to pack */
> >
> > /* --- cacheline 1 boundary (64 bytes) --- */
> > struct rhashtable rhashtable; /* 64 136 */
> >
> > /* XXX 56 bytes hole, try to pack */
>
> This one is strange, there should be no hole. Will take a look.
Ok, so this one is due to calling class__find_holes() multiple times
on the same struct. Last member's bit_hole/hole are not zeroed out. I
have a fix, which I'll add to my last patchset. I'll also drop last
patch (partially disabling padding recalc for bitfield). Will send
soon.
>
> >
> > /* size: 200, cachelines: 4, members: 7 */
> > /* sum members: 176, holes: 1, sum holes: 80 */
> > /* last cacheline: 8 bytes */
> >
> > /* BRAIN FART ALERT! 200 bytes != 176 (member bytes) + 0 (member bits) + 80 (byte holes) + 0 (bit holes), diff = -448 bits */
> > }; /* saved 120 bytes and 1 cacheline! */
> > [acme@quaco pahole]$
> >
> > So I'll apply this one and aim for tagging v1.13. Then we get back and
> > get the bitfield reorg nailed.
>
> Sounds good, thanks!
>
> >
> > - Arnaldo
> >
> > diff --git a/dwarves_reorganize.c b/dwarves_reorganize.c
> > index f1fc2158d03f..3e4cd9c66b82 100644
> > --- a/dwarves_reorganize.c
> > +++ b/dwarves_reorganize.c
> > @@ -226,12 +226,20 @@ static struct class_member *
> > return NULL;
> > }
> >
> > -static void class__move_member(struct class *class, struct class_member *dest,
> > - struct class_member *from, const struct cu *cu,
> > - int from_padding, const int verbose, FILE *fp)
> > +static bool class__move_member(struct class *class, struct class_member *dest,
> > + struct class_member *from, const struct cu *cu,
> > + int from_padding, const int verbose, FILE *fp)
> > {
> > const size_t from_size = from->byte_size;
> > const size_t dest_size = dest->byte_size;
> > +
> > +#ifndef BITFIELD_REORG_ALGORITHMS_ENABLED
> > + /*
> > + * For now refuse to move a bitfield, we need to first fixup some BRAIN FARTs
> > + */
> > + if (from->bitfield_size != 0)
> > + return false;
> > +#endif
> > const bool from_was_last = from->tag.node.next == class__tags(class);
> > struct class_member *tail_from = from;
> > struct class_member *from_prev = list_entry(from->tag.node.prev,
> > @@ -347,6 +355,8 @@ static void class__move_member(struct class *class, struct class_member *dest,
> > class__fprintf(class, cu, fp);
> > fputc('\n', fp);
> > }
> > +
> > + return true;
> > }
> >
> > static void class__move_bit_member(struct class *class, const struct cu *cu,
> > @@ -743,11 +753,11 @@ void class__reorganize(struct class *class, const struct cu *cu,
> > size_t alignment_size;
> >
> > class__find_holes(class);
> > +#ifdef BITFIELD_REORG_ALGORITHMS_ENABLED
> > class__fixup_member_types(class, cu, verbose, fp);
> > -
> > while (class__demote_bitfields(class, cu, verbose, fp))
> > class__reorganize_bitfields(class, cu, verbose, fp);
> > -
> > +#endif
> > /* Now try to combine holes */
> > restart:
> > alignment_size = 0;
> > @@ -807,10 +817,8 @@ restart:
> > * kernel.
> > */
> > if (brother_prev != member) {
> > - class__move_member(class, member,
> > - brother, cu, 0,
> > - verbose, fp);
> > - goto restart;
> > + if (class__move_member(class, member, brother, cu, 0, verbose, fp))
> > + goto restart;
> > }
> > }
> > /*
> > @@ -824,9 +832,8 @@ restart:
> > member != last_member &&
> > last_member->byte_size != 0 &&
> > last_member->byte_size <= member->hole) {
> > - class__move_member(class, member, last_member,
> > - cu, 1, verbose, fp);
> > - goto restart;
> > + if (class__move_member(class, member, last_member, cu, 1, verbose, fp))
> > + goto restart;
> > }
> > }
> > }
> > @@ -852,10 +859,8 @@ restart:
> > * kernel.
> > */
> > if (brother_prev != member) {
> > - class__move_member(class, member,
> > - brother, cu, 0,
> > - verbose, fp);
> > - goto restart;
> > + if (class__move_member(class, member, brother, cu, 0, verbose, fp))
> > + goto restart;
> > }
> > }
> > }
>
> Looks good.
>
> Acked-by: Andrii Nakryiko <andriin@xxxxxx>
