Re: [PATCH net 1/1] s390/ism: fix receive message buffer allocation

[Gerd Bayer <gbayer@linux.ibm.com>]

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 |
__GFP_NORETRY,
+					  get_order(dmb->dmb_len)));
+
 	if (!dmb->cpu_addr) {
 		rc = -ENOMEM;
 		goto out_bit;
-- 
2.44.0