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

[Niklas Schnelle <schnelle@linux.ibm.com>]

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;