Re: [PATCH bpf-next v2 08/10] xsk: Support UMEM chunk_size > PAGE_SIZE

From: Kal Cutter Conley <kal.conley@dectris.com>
To: Magnus Karlsson <magnus.karlsson@gmail.com>
Cc: "Björn Töpel" <bjorn@kernel.org>,
	"Magnus Karlsson" <magnus.karlsson@intel.com>,
	"Maciej Fijalkowski" <maciej.fijalkowski@intel.com>,
	"Jonathan Lemon" <jonathan.lemon@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	"Eric Dumazet" <edumazet@google.com>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Paolo Abeni" <pabeni@redhat.com>,
	"Jonathan Corbet" <corbet@lwn.net>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"Jesper Dangaard Brouer" <hawk@kernel.org>,
	"John Fastabend" <john.fastabend@gmail.com>,
	netdev@vger.kernel.org, bpf@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH bpf-next v2 08/10] xsk: Support UMEM chunk_size > PAGE_SIZE
Date: Tue, 4 Apr 2023 12:33:58 +0200	[thread overview]
Message-ID: <CAHApi-=ui3JofMr7y+LvuYkXCU=h7vGiKXsfuV5gog-02u-u+Q@mail.gmail.com> (raw)
In-Reply-To: <CAJ8uoz0SEkcXQuoqYd94GreJqpCxQuf1QVgm9=Um6Wqk=s8GBw@mail.gmail.com>

> > > Is not the max 64K as you test against XDP_UMEM_MAX_CHUNK_SIZE in
> > > xdp_umem_reg()?
> >
> > The absolute max is 64K. In the case of HPAGE_SIZE < 64K, then it
> > would be HPAGE_SIZE.
>
> Is there such a case when HPAGE_SIZE would be less than 64K? If not,
> then just write 64K.

Yes. While most platforms have HPAGE_SIZE defined to a compile-time
constant >= 64K (very often 2M) there are platforms (at least ia64 and
powerpc) where the hugepage size is configured at boot. Specifically,
in the case of Itanium (ia64), the hugepage size may be configured at
boot to any valid page size > PAGE_SIZE (e.g. 8K). See:
https://elixir.bootlin.com/linux/latest/source/arch/ia64/mm/hugetlbpage.c#L159

>
> > > >  static int xdp_umem_pin_pages(struct xdp_umem *umem, unsigned long address)
> > > >  {
> > > > +#ifdef CONFIG_HUGETLB_PAGE
> > >
> > > Let us try to get rid of most of these #ifdefs sprinkled around the
> > > code. How about hiding this inside xdp_umem_is_hugetlb() and get rid
> > > of these #ifdefs below? Since I believe it is quite uncommon not to
> > > have this config enabled, we could simplify things by always using the
> > > page_size in the pool, for example. And dito for the one in struct
> > > xdp_umem. What do you think?
> >
> > I used #ifdef for `page_size` in the pool for maximum performance when
> > huge pages are disabled. We could also not worry about optimizing this
> > uncommon case though since the performance impact is very small.
> > However, I don't find the #ifdefs excessive either.
>
> Keep them to a minimum please since there are few of them in the
> current code outside of some header files. And let us assume that
> CONFIG_HUGETLB_PAGE is the common case.
>

Would you be OK if I just remove the ones from xsk_buff_pool? I think
the code in xdp_umem.c is quite readable and the #ifdefs are really
only used in xdp_umem_pin_pages.