All of lore.kernel.org
 help / color / mirror / Atom feed
From: David Laight <David.Laight@ACULAB.COM>
To: 'Stephen Hemminger' <stephen@networkplumber.org>,
	Johannes Berg <johannes@sipsolutions.net>
Cc: Jakub Kicinski <kuba@kernel.org>,
	Nicolas Dichtel <nicolas.dichtel@6wind.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"fw@strlen.de" <fw@strlen.de>,
	"pablo@netfilter.org" <pablo@netfilter.org>,
	"jiri@resnulli.us" <jiri@resnulli.us>,
	"mkubecek@suse.cz" <mkubecek@suse.cz>,
	"aleksander.lobakin@intel.com" <aleksander.lobakin@intel.com>,
	Thomas Haller <thaller@redhat.com>
Subject: RE: [RFC] netlink: add variable-length / auto integers
Date: Thu, 12 Oct 2023 09:26:06 +0000	[thread overview]
Message-ID: <e55db83d221d4b5c9fce899cc60cb378@AcuMS.aculab.com> (raw)
In-Reply-To: <20231011094550.7837d43a@hermes.local>

From: Stephen Hemminger
> Sent: 11 October 2023 17:46
> 
> On Wed, 11 Oct 2023 18:01:49 +0200
> Johannes Berg <johannes@sipsolutions.net> wrote:
> 
> > On Wed, 2023-10-11 at 08:52 -0700, Jakub Kicinski wrote:
> >
> > > > > > Even for arches which don't have good unaligned access - I'd think
> > > > > > that access aligned to 4B *is* pretty efficient, and that's all
> > > > > > we need. Plus kernel deals with unaligned input. Why can't user space?
> > > > >
> > > > > Hmm. I have a vague recollection that it was related to just not doing
> > > > > it - the kernel will do get_unaligned() or similar, but userspace if it
> > > > > just accesses it might take a trap on some architectures?
> > > > >
> > > > > But I can't find any record of this in public discussions, so ...
> > > > If I remember well, at this time, we had some (old) architectures that triggered
> > > > traps (in kernel) when a 64-bit field was accessed and unaligned. Maybe a mix
> > > > between 64-bit kernel / 32-bit userspace, I don't remember exactly. The goal was
> > > > to align u64 fields on 8 bytes.
> > >
> > > Reading the discussions I think we can chalk the alignment up
> > > to "old way of doing things". Discussion was about stats64,
> > > if someone wants to access stats directly in the message then yes,
> > > they care a lot about alignment.
> > >
> > > Today we try to steer people towards attr-per-field, rather than
> > > dumping structs. Instead of doing:
> > >
> > > 	struct stats *stats = nla_data(attr);
> > > 	print("A: %llu", stats->a);
> > >
> > > We will do:
> > >
> > > 	print("A: %llu", nla_get_u64(attrs[NLA_BLA_STAT_A]));
> >
> > Well, yes, although the "struct stats" part _still_ even exists in the
> > kernel, we never fixed that with the nla_put_u64_64bit() stuff, that was
> > only for something that does
> >
> > 	print("A: %" PRIu64, *(uint64_t *)nla_data(attrs[NLA_BLA_STAT_A]));
> >
> > > Assuming nla_get_u64() is unalign-ready the problem doesn't exist.
> >
> > Depends on the library, but at least for libnl that's true since ever.
> > Same for libmnl and libnl-tiny. So I guess it only ever hit hand-coded
> > implementations.
> 
> Quick check of iproute2 shows places where stats are directly
> mapped without accessors. One example is print_mpls_stats().

You 'just' need to use the 64bit type that has __attribute__((aligned(4))).
The same is true for the code that reads/writes the value.
Better than passing by address and using memcpy();

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


  reply	other threads:[~2023-10-12  9:26 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-10-11  0:33 [RFC] netlink: add variable-length / auto integers Jakub Kicinski
2023-10-11  3:20 ` kernel test robot
2023-10-11 13:11 ` Johannes Berg
2023-10-11 14:03   ` Nicolas Dichtel
2023-10-11 15:52     ` Jakub Kicinski
2023-10-11 16:01       ` Johannes Berg
2023-10-11 16:45         ` Stephen Hemminger
2023-10-12  9:26           ` David Laight [this message]
2023-10-12  6:47       ` Nicolas Dichtel
2023-10-11 16:08   ` Jakub Kicinski
2023-10-11 16:16     ` Johannes Berg
2023-10-11 16:19       ` Jakub Kicinski
2023-10-11 13:46 ` Jiri Pirko
2023-10-11 16:16   ` Jakub Kicinski
2023-10-11 16:21     ` Johannes Berg
2023-10-11 16:34       ` Jakub Kicinski
2023-10-11 16:37         ` Johannes Berg
2023-10-11 17:01     ` Jiri Pirko
2023-10-11 20:21       ` Jakub Kicinski
2023-10-12 12:36 ` kernel test robot
2023-10-14 12:35 ` kernel test robot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e55db83d221d4b5c9fce899cc60cb378@AcuMS.aculab.com \
    --to=david.laight@aculab.com \
    --cc=aleksander.lobakin@intel.com \
    --cc=fw@strlen.de \
    --cc=jiri@resnulli.us \
    --cc=johannes@sipsolutions.net \
    --cc=kuba@kernel.org \
    --cc=mkubecek@suse.cz \
    --cc=netdev@vger.kernel.org \
    --cc=nicolas.dichtel@6wind.com \
    --cc=pablo@netfilter.org \
    --cc=stephen@networkplumber.org \
    --cc=thaller@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.