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.
>
> /* 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>
