| Message ID | 1461339084-3849-10-git-send-email-nicolas.dichtel@6wind.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On 23/04/16 01:31, Nicolas Dichtel wrote: > Goal of this patch is to use the new libnl API to align netlink attribute > when needed. > The layout of the netlink message will be a bit different after the patch, > because the padattr (TASKSTATS_TYPE_STATS) will be inside the nested > attribute instead of before it. > > Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> The layout will change/break user space -- I've not tested the patch though.. Balbir Singh.
Le 27/04/2016 03:14, Balbir Singh a écrit : > > > On 23/04/16 01:31, Nicolas Dichtel wrote: >> Goal of this patch is to use the new libnl API to align netlink attribute >> when needed. >> The layout of the netlink message will be a bit different after the patch, >> because the padattr (TASKSTATS_TYPE_STATS) will be inside the nested >> attribute instead of before it. >> >> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> > > The layout will change/break user space -- I've not tested the patch though.. Sigh. I quote David: "All userspace components using netlink should always ignore attributes they do not recognize in dumps. This is one of the most basic principles of netlink" Do you have some pointers so I can made some tests? Regards, Nicolas
On 27/04/16 17:29, Nicolas Dichtel wrote: > Le 27/04/2016 03:14, Balbir Singh a écrit : >> >> >> On 23/04/16 01:31, Nicolas Dichtel wrote: >>> Goal of this patch is to use the new libnl API to align netlink attribute >>> when needed. >>> The layout of the netlink message will be a bit different after the patch, >>> because the padattr (TASKSTATS_TYPE_STATS) will be inside the nested >>> attribute instead of before it. >>> >>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> >> >> The layout will change/break user space -- I've not tested the patch though.. > Sigh. > > I quote David: > "All userspace components using netlink should always ignore attributes > they do not recognize in dumps. > > This is one of the most basic principles of netlink" > > Do you have some pointers so I can made some tests? > Please try https://www.kernel.org/doc/Documentation/accounting/getdelays.c iotop uses it as well. My concern is ABI breakage of user space. Balbir Singh
Le 27/04/2016 14:29, Balbir Singh a écrit : [snip] > Please try > > https://www.kernel.org/doc/Documentation/accounting/getdelays.c A patch follows this mail to fix that. > > iotop uses it as well. My concern is ABI breakage of user space. My test is ok here, I didn't see a problem. Code review (from here http://repo.or.cz/w/iotop.git) is also ok. Regards, Nicolas
From: Balbir Singh <bsingharora@gmail.com> Date: Wed, 27 Apr 2016 22:29:22 +1000 > My concern is ABI breakage of user space. The "ABI" is that unrecognized attributes must be silently ignored by userspace.
diff --git a/kernel/taskstats.c b/kernel/taskstats.c index 21f82c29c914..b3f05ee20d18 100644 --- a/kernel/taskstats.c +++ b/kernel/taskstats.c @@ -357,10 +357,6 @@ static int parse(struct nlattr *na, struct cpumask *mask) return ret; } -#if defined(CONFIG_64BIT) && !defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) -#define TASKSTATS_NEEDS_PADDING 1 -#endif - static struct taskstats *mk_reply(struct sk_buff *skb, int type, u32 pid) { struct nlattr *na, *ret; @@ -370,29 +366,6 @@ static struct taskstats *mk_reply(struct sk_buff *skb, int type, u32 pid) ? TASKSTATS_TYPE_AGGR_PID : TASKSTATS_TYPE_AGGR_TGID; - /* - * The taskstats structure is internally aligned on 8 byte - * boundaries but the layout of the aggregrate reply, with - * two NLA headers and the pid (each 4 bytes), actually - * force the entire structure to be unaligned. This causes - * the kernel to issue unaligned access warnings on some - * architectures like ia64. Unfortunately, some software out there - * doesn't properly unroll the NLA packet and assumes that the start - * of the taskstats structure will always be 20 bytes from the start - * of the netlink payload. Aligning the start of the taskstats - * structure breaks this software, which we don't want. So, for now - * the alignment only happens on architectures that require it - * and those users will have to update to fixed versions of those - * packages. Space is reserved in the packet only when needed. - * This ifdef should be removed in several years e.g. 2012 once - * we can be confident that fixed versions are installed on most - * systems. We add the padding before the aggregate since the - * aggregate is already a defined type. - */ -#ifdef TASKSTATS_NEEDS_PADDING - if (nla_put(skb, TASKSTATS_TYPE_NULL, 0, NULL) < 0) - goto err; -#endif na = nla_nest_start(skb, aggr); if (!na) goto err; @@ -401,7 +374,8 @@ static struct taskstats *mk_reply(struct sk_buff *skb, int type, u32 pid) nla_nest_cancel(skb, na); goto err; } - ret = nla_reserve(skb, TASKSTATS_TYPE_STATS, sizeof(struct taskstats)); + ret = nla_reserve_64bit(skb, TASKSTATS_TYPE_STATS, + sizeof(struct taskstats), TASKSTATS_TYPE_NULL); if (!ret) { nla_nest_cancel(skb, na); goto err; @@ -500,10 +474,9 @@ static size_t taskstats_packet_size(void) size_t size; size = nla_total_size(sizeof(u32)) + - nla_total_size(sizeof(struct taskstats)) + nla_total_size(0); -#ifdef TASKSTATS_NEEDS_PADDING - size += nla_total_size(0); /* Padding for alignment */ -#endif + nla_total_size_64bit(sizeof(struct taskstats)) + + nla_total_size(0); + return size; }
Goal of this patch is to use the new libnl API to align netlink attribute when needed. The layout of the netlink message will be a bit different after the patch, because the padattr (TASKSTATS_TYPE_STATS) will be inside the nested attribute instead of before it. Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> --- kernel/taskstats.c | 37 +++++-------------------------------- 1 file changed, 5 insertions(+), 32 deletions(-)