Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 29 Oct 2019 10:25:33 +0800
From:   Ming Lei <ming.lei@redhat.com>
To:     Sultan Alsawaf <sultan@kerneltoast.com>
Cc:     Jason Gunthorpe <jgg@ziepe.ca>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        Thomas Hellstrom <thellstrom@vmware.com>,
        Palmer Dabbelt <palmer@sifive.com>,
        Sakari Ailus <sakari.ailus@linux.intel.com>,
        Gal Pressman <galpress@amazon.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] scatterlist: Speed up for_each_sg() loop macro
Message-ID: <20191029022533.GE22088@ming.t460p>
References: <20191025213359.7538-1-sultan@kerneltoast.com>
MIME-Version: 1.0
In-Reply-To: <20191025213359.7538-1-sultan@kerneltoast.com>
User-Agent: Mutt/1.12.1 (2019-06-15)
Content-Type: text/plain; charset=WINDOWS-1252
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Oct 25, 2019 at 02:33:58PM -0700, Sultan Alsawaf wrote:
> From: Sultan Alsawaf <sultan@kerneltoast.com>
>=20
> Scatterlists are chained in predictable arrays of up to
> SG_MAX_SINGLE_ALLOC sg structs in length. Using this knowledge, speed up
> for_each_sg() by using constant operations to determine when to simply
> increment the sg pointer by one or get the next sg array in the chain.
>=20
> Rudimentary measurements with a trivial loop body show that this yields
> roughly a 2x performance gain.
>=20
> The following simple test module proves the correctness of the new loop
> definition by testing all the different edge cases of sg chains:
> #include <linux/module.h>
> #include <linux/scatterlist.h>
> #include <linux/slab.h>
>=20
> static int __init test_for_each_sg(void)
> {
> =09static const gfp_t gfp_flags =3D GFP_KERNEL | __GFP_NOFAIL;
>         struct scatterlist *sg;
>         struct sg_table *table;
>         long old =3D 0, new =3D 0;
>         unsigned int i, nents;
>=20
>         table =3D kmalloc(sizeof(*table), gfp_flags);
>         for (nents =3D 1; nents <=3D 3 * SG_MAX_SINGLE_ALLOC; nents++) {
>                 BUG_ON(sg_alloc_table(table, nents, gfp_flags));
>                 for (sg =3D table->sgl; sg; sg =3D sg_next(sg))
>                         old ^=3D (long)sg;
>                 for_each_sg(table->sgl, sg, nents, i)
>                         new ^=3D (long)sg;
>                 sg_free_table(table);
>         }
>=20
>         BUG_ON(old !=3D new);
>         kfree(table);
>         return 0;
> }
> module_init(test_for_each_sg);
>=20
> Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com>
> ---
>  include/linux/scatterlist.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>=20
> diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
> index 556ec1ea2574..73f7fd6702d7 100644
> --- a/include/linux/scatterlist.h
> +++ b/include/linux/scatterlist.h
> @@ -146,7 +146,10 @@ static inline void sg_set_buf(struct scatterlist *sg=
, const void *buf,
>   * Loop over each sg element, following the pointer to a new list if nec=
essary
>   */
>  #define for_each_sg(sglist, sg, nr, __i)=09\
> -=09for (__i =3D 0, sg =3D (sglist); __i < (nr); __i++, sg =3D sg_next(sg=
))
> +=09for (__i =3D 0, sg =3D (sglist); __i < (nr);=09=09\
> +=09     likely(++__i % (SG_MAX_SINGLE_ALLOC - 1) ||=09\
> +=09=09    (__i + 1) >=3D (nr)) ? sg++ :=09=09=09\
> +=09=09    (sg =3D sg_chain_ptr(sg + 1)))
> =20

sg_alloc_table_chained() may put a small sglist as the first chunk, then
chained with big one, and your patch breaks such usage.


Thanks,
Ming