On Fri, 2024-04-05 at 12:42 +0200, Gerd Bayer wrote:
> On Fri, 2024-04-05 at 08:49 +0200, Christoph Hellwig wrote:
> > On Thu, Apr 04, 2024 at 01:10:20PM +0200, Gerd Bayer wrote:
> > > > Why can't you use get_free_pages() (or similar) here? (possibly
> > > > rounding up to the relevant page_aligned size).
> > >
> > > Thanks Paolo for your suggestion. However, I wanted to stay as
> > > close to the implementation pre [1] - that used to use __GFP_COMP,
> > > too. I'd rather avoid to change interfaces from "cpu_addr" to
> > > "struct page*" at this point. In the long run, I'd like to drop the
> > > requirement for
> >
> > The right interface actually is to simply use folio_alloc, which adds
> > __GFP_COMP and is a fully supported and understood interface. You can
> > just convert the folio to a kernel virtual address using
> > folio_address() right after allocating it.
>
> Thanks for pointing me to folios.
> After a good night's sleep, I figured that I was thinking too
> complicated when I dismissed Paolo's suggestion.
>
> > (get_free_pages also retunrs a kernel virtual address, just awkwardly
> > as an unsigned long. In doubt don't use this interface for new
> > code..)
> >
> > > compound pages entirely, since that *appears* to exist primarily
> > > for a
> > > simplified handling of the interface to splice_to_pipe() in
> > > net/smc/smc_rx.c. And of course there might be performance
> > > implications...
> >
> > While compounds pages might sound awkward, they are the new normal in
> > form of folios. So just use folios.
>
> With the following fixup, my tests were just as successful.
> I'll send that out as a v2.
>
> Thank you, Christoph and Paolo!
>
>
>
> diff --git a/drivers/s390/net/ism_drv.c b/drivers/s390/net/ism_drv.c
> index 25911b887e5e..affb05521e14 100644
> --- a/drivers/s390/net/ism_drv.c
> +++ b/drivers/s390/net/ism_drv.c
> @@ -14,8 +14,8 @@
> #include <linux/err.h>
> #include <linux/ctype.h>
> #include <linux/processor.h>
> -#include <linux/dma-direction.h>
> -#include <linux/gfp_types.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/mm.h>
>
> #include "ism.h"
>
> @@ -296,7 +296,7 @@ static void ism_free_dmb(struct ism_dev *ism,
> struct ism_dmb *dmb)
> clear_bit(dmb->sba_idx, ism->sba_bitmap);
> dma_unmap_page(&ism->pdev->dev, dmb->dma_addr, dmb->dmb_len,
> DMA_FROM_DEVICE);
> - kfree(dmb->cpu_addr);
> + folio_put(virt_to_folio(dmb->cpu_addr));
> }
>
> static int ism_alloc_dmb(struct ism_dev *ism, struct ism_dmb *dmb)
> @@ -319,8 +319,11 @@ static int ism_alloc_dmb(struct ism_dev *ism,
> struct ism_dmb *dmb)
> test_and_set_bit(dmb->sba_idx, ism->sba_bitmap))
> return -EINVAL;
>
> - dmb->cpu_addr = kmalloc(dmb->dmb_len, GFP_KERNEL |
> __GFP_NOWARN |
> - __GFP_COMP | __GFP_NOMEMALLOC |
> __GFP_NORETRY);
> + dmb->cpu_addr =
> + folio_address(folio_alloc(GFP_KERNEL | __GFP_NOWARN |
> + __GFP_NOMEMALLOC |
Personally I'd go with a temporary variable here if only to make the
lines a bit shorter and easier to read. I also think above is not
correct for allocation failure since folio_address() accesses folio-
>page without first checking for NULL. So I'm guessing the NULL check
needs to move and be done on the temporary struct folio*.
> __GFP_NORETRY,
> + get_order(dmb->dmb_len)));
> +
> if (!dmb->cpu_addr) {
> rc = -ENOMEM;
> goto out_bit;
