[ofa-general] OFED, the backported <linux/scatterlist.h> header and sg_init_table()

[Jon Mason]

On Sun, May 03, 2009 at 05:36:53PM +0200, Bart Van Assche wrote:
> On Sun, May 3, 2009 at 3:04 PM, Jack Morgenstein
> <jackm at dev.mellanox.co.il> wrote:
> > On Saturday 02 May 2009 14:46, Bart Van Assche wrote:
> >> Hello,
> >>
> >> Yesterday I installed OFED-1.4.1-rc4 on a CentOS 5.3 system and started
> >> looking at the backported kernel headers. I found the following in the
> >> header file
> >> /usr/src/ofa_kernel-1.4.1/kernel_addons/backport/2.6.18-EL5.3/include/linux/scatterlist.h:
> >>
> >> #define sg_init_table(a, b)
> >>
> >> Or: sg_init_table() is defined to do nothing. I was expecting the following
> >> however:
> >>
> >> #define sg_init_table(sgl, nents) memset(sgl, 0, sizeof(*sgl) * nents);
> >>
> >> The sg_init_table() function is implemented in e.g. 2.6.29 as follows:
> >>
> >> void sg_init_table(struct scatterlist *sgl, unsigned int nents)
> >> {
> >>         memset(sgl, 0, sizeof(*sgl) * nents);
> >> #ifdef CONFIG_DEBUG_SG
> >>         {
> >>                 unsigned int i;
> >>                 for (i = 0; i < nents; i++)
> >>                         sgl[i].sg_magic = SG_MAGIC;
> >>         }
> >> #endif
> >>         sg_mark_end(&sgl[nents - 1]);
> >> }
> >>
> >> Does anyone know why sg_init_table() is defined such that it does nothing in
> >> the backported OFED headers ?
> >
> > I checked this more carefully.
> > Use of sg_init_table was introduced in 2.6.24 by Jens Axboe, in commit
> > 45711f1af6eff1a6d010703b4862e0d2b9afd056. (see chunks for core/umem.c)
> >
> > Before this, no initialization was done on the sg page_list, and we had no
> > problems.  When doing the backport, then, I simply made this a NOP.
> > I'm not convinced that sg_init_table needs to be implemented in kernels earlier
> > than 2.6.24, since this call is not replacing anything (e.g., a kzalloc), and
> > the page list was not previously zeroed out before usage.
> >
> > What do you think?
> 
> My opinion is that it is really dangerous and confusing to have one
> version of the sg_init_table() macro that performs initialization and
> another version that does not. As an example, the OFED source file
> net/sunrpc/xdr.c invokes sg_init_table(). When this code is compiled
> against e.g. a 2.6.27 kernel, invoking sg_init_table() will
> initialize the sg-list properly because in this case the
> sg_init_table() included with the 2.6.27 kernel is used. When this
> code is compiled against e.g. an RHEL 5.3 kernel, invoking the
> sg_init_table() macro will have no effect because the sg_init_table()
> macro from OFED's backported header files is used. Is this effect
> really desired ?

What's even worse is that sg_init_table is already defined in the
RHEL5.3 headers.  When coding up a header cleanup patch for RHEL5.3, I
noticed it was already defined in linux/ncrypto.h.  Also, it's there for
RHEL5.2 (and a few older kernels).

I should have the patch out today for review.

Thanks,
Jon



> 
> Bart.
> _______________________________________________
> general mailing list
> general at lists.openfabrics.org
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
> 
> To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general