All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Jakub Bogusz <qboosh@pld-linux.org>
Cc: bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>
Subject: Re: [PATCH] fix libbpf hashmap with size_t shorter than long long
Date: Tue, 23 Jun 2020 12:40:02 -0700	[thread overview]
Message-ID: <CAEf4BzbKo1-61emwL5nWHRVTeabvedZC6QX01u=pthgkcL3iag@mail.gmail.com> (raw)
In-Reply-To: <20200623192917.GA6342@mail>

On Tue, Jun 23, 2020 at 12:29 PM Jakub Bogusz <qboosh@pld-linux.org> wrote:
>
> On Mon, Jun 22, 2020 at 10:44:56PM -0700, Andrii Nakryiko wrote:
> > On Sun, Jun 21, 2020 at 7:34 AM Jakub Bogusz <qboosh@pld-linux.org> wrote:
> > >
> > > Hello,
> > >
> > > I noticed that _bpftool crashes when building kernel tools (5.7.x) for
> > > 32-bit targets because in libbpf hashmap implementation hash_bits()
> > > function returning numbers exceeding hashmap buckets capacity.
> > >
> > > Attached patch fixes this problem.
> > >
> >
> > Thanks! But this was already fixed by Arnaldo Carvalho de Melo <acme@kernel.org>
> > in 8ca8d4a84173 ("libbpf: Define __WORDSIZE if not available").
>
> No, it's not:
> This change worked around __WORDSIZE not always being available.
>
> But the issue on (I)LP32 platforms is that 64-bit value is shifted by
> (32-bits) instead of (64-bits).
>
> (__SIZEOF_LONG__ * 8) is 32 on such architectures (i686, arm).
> I used __SIZEOF_LONG_LONG__ to get proper bit shift both on (I)LP32 and
> LP64 architectures.
>

Ah, I see. I actually mentioned __SIZEOF_ constants on the original
fix patch. But I think in this case it has to use __SIZEOF_SIZE_T,
which on 32-bit should be 4, right?


> Should I provide an updated patch to apply on top of acme change?

Yes, that would be good. But I think there is no need to penalize
32-bit arches with use of 64-bit long longs, and instead it's better
to use #ifdef for 32-bit case vs 64-bit case. The multiplication
constant will change, of course, should be 2654435769. I'd appreciate
it if you can do the patch, thanks!


>
>
> Regards,
>
> --
> Jakub Bogusz    http://qboosh.pl/

  reply	other threads:[~2020-06-23 19:40 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-21 14:25 [PATCH] fix libbpf hashmap with size_t shorter than long long Jakub Bogusz
2020-06-23  5:44 ` Andrii Nakryiko
2020-06-23 19:29   ` Jakub Bogusz
2020-06-23 19:40     ` Andrii Nakryiko [this message]
2020-06-27  9:07       ` Jakub Bogusz
2020-06-27 20:25         ` Andrii Nakryiko

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='CAEf4BzbKo1-61emwL5nWHRVTeabvedZC6QX01u=pthgkcL3iag@mail.gmail.com' \
    --to=andrii.nakryiko@gmail.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=qboosh@pld-linux.org \
    /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.