Re: 2.6.24-rc6-mm1

[Torsten Kaiser]

On Jan 6, 2008 4:28 AM, FUJITA Tomonori <tomof@xxxxxxx> wrote:
> On Sat, 5 Jan 2008 17:25:24 -0800
> Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> > On Sat, 5 Jan 2008 23:10:17 +0100 "Torsten Kaiser" <just.for.lkml@xxxxxxxxxxxxxx> wrote:
> > > But the cause of my mail is the following question:
> > > Regarding my "iommu-sg-merging-patches are new in -rc3-mm and could be
> > > the cause"-suspicion I looked at these patches and came across these
> > > hunks:
> > >
> > > This is removed from arch/x86/lib/bitstr_64.c:
> > > -/* Find string of zero bits in a bitmap */
> > > -unsigned long
> > > -find_next_zero_string(unsigned long *bitmap, long start, long nbits, int len)
> > > -{
> > > -       unsigned long n, end, i;
> > > -
> > > - again:
> > > -       n = find_next_zero_bit(bitmap, nbits, start);
> > > -       if (n == -1)
> > > -               return -1;
> > > -
> > > -       /* could test bitsliced, but it's hardly worth it */
> > > -       end = n+len;
> > > -       if (end > nbits)
> > > -               return -1;
> > > -       for (i = n+1; i < end; i++) {
> > > -               if (test_bit(i, bitmap)) {
> > > -                       start = i+1;
> > > -                       goto again;
> > > -               }
> > > -       }
> > > -       return n;
> > > -}
> > >
> > > This is added to lib/iommu-helper.c:
> > > +static unsigned long find_next_zero_area(unsigned long *map,
> > > +                                        unsigned long size,
> > > +                                        unsigned long start,
> > > +                                        unsigned int nr)
> > > +{
> > > +       unsigned long index, end, i;
> > > +again:
> > > +       index = find_next_zero_bit(map, size, start);
> > > +       end = index + nr;
> > > +       if (end > size)
> > > +               return -1;
> > > +       for (i = index + 1; i < end; i++) {
> > > +               if (test_bit(i, map)) {
> > > +                       start = i+1;
> > > +                       goto again;
> > > +               }
> > > +       }
> > > +       return index;
> > > +}
> > >
> > > The old version checks, if find_next_zero_bit returns -1, the new
> > > version doesn't do this.
> > > Is this intended and can find_next_zero_bit never fail?
> > > Hmm... but in the worst case it should only loop forever if I'm
> > > reading this right (index = -1 => for-loop counts from 0 to nr, if any
> > > bit is set it will jump to "again:" and if the next call to
> > > find_next_zero_bit also fails, its an endless loop)
>
> find_next_zero_bit returns -1?
>
> It seems that x86_64 doesn't.

I'm sorry. I didn't look into find_next_zero_bit, I only noted that
the old version did check for -1 and the new one didn't.
Obviously the old check was superfluous.

> POWER and SPARC64 IOMMUs use
> find_next_zero_bit too but both doesn't check if find_next_zero_bit
> returns -1. If find_next_zero_bit fails, it returns size. So it
> doesn't leads to an endless loop.

Yes, this can't happen. It was a wrong assumption on my part.

> But this patch has other bugs that break POWER IOMMUs.
>
> If you use the IOMMUs on POWER, please try the following patch:

I'm using CONFIG_GART_IOMMU=y on x86_64.

> http://www.mail-archive.com/linux-scsi@xxxxxxxxxxxxxxx/msg12702.html

I also noted the line "index = (index + align_mask) & ~align_mask;" in
iommu_area_alloc() and didn't understand what this was trying to do
and how this should work, but as arch/x86/kernel/pci-gart_64.c always
uses 0 as align_mask I just ignored it.

I will applie your patch and see if this hunk from
find_next_zero_area() makes a difference:

       end = index + nr;
-       if (end > size)
+       if (end >= size)
                return -1;
-       for (i = index + 1; i < end; i++) {
+       for (i = index; i < end; i++) {
                if (test_bit(i, map)) {

Torsten
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/