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