parisc scatterlist doesn't want page/offset

parisc scatterlist doesn't want page/offset

[Grant Grundler]

Hi all,
Could someone point me to any discussion about adding
page/offset to struct scatterlist?

To me, it looks like a half-assed step to support DMA to HIGHMEM
on 32-bit arches.  TBH, I'd like to see page/offset replace
address in the pci_map* interfaces and struct scatterlist.
But then replace it across the board so the DMA mapping code
doesn't have to decide which field to use (KISS). This really
belongs in 2.5 kernel.

parisc has just merged up to 2.4.14 and picked up the following
macros from arch/sparc:
/* No highmem on parisc, plus we have an IOMMU, so mapping pages is easy. */
#define pci_map_page(dev, page, off, size, dir) \
        pci_map_single(dev, (page_address(page) + (off)), size, dir) 
#define pci_unmap_page(dev,addr,sz,dir) pci_unmap_single(dev,addr,sz,dir)

afaict, parisc doesn't need page/offset in struct scatterlist.
As an interim solution, I've appended a patch that I'm hoping
is acceptable (or at least a starting point).
Thoughts?

thanks,
grant

ps. I'm trying to be constructive - it's a bit difficult after
  putting up with davem ranting about how the DMA mapping interface
  in 2.4 was frozen.

Index: drivers/scsi/scsi_merge.c
===================================================================
RCS file: /var/cvs/linux/drivers/scsi/scsi_merge.c,v
retrieving revision 1.10
diff -u -p -r1.10 scsi_merge.c
--- drivers/scsi/scsi_merge.c	2001/11/09 23:36:24	1.10
+++ drivers/scsi/scsi_merge.c	2001/11/16 07:26:36
@@ -943,7 +943,9 @@ __inline static int __init_io(Scsi_Cmnd 
 		}
 		count++;
 		sgpnt[count - 1].address = bh->b_data;
+#ifdef CONFIG_HIGHMEM
 		sgpnt[count - 1].page = NULL;
+#endif
 		sgpnt[count - 1].length += bh->b_size;
 		if (!dma_host) {
 			SCpnt->request_bufflen += bh->b_size;
Index: drivers/scsi/sg.c
===================================================================
RCS file: /var/cvs/linux/drivers/scsi/sg.c,v
retrieving revision 1.12
diff -u -p -r1.12 sg.c
--- drivers/scsi/sg.c	2001/11/09 23:36:24	1.12
+++ drivers/scsi/sg.c	2001/11/16 07:26:37
@@ -1544,7 +1544,9 @@ static int sg_build_dir(Sg_request * srp
 	num = (rem_sz > (PAGE_SIZE - offset)) ? (PAGE_SIZE - offset) :
 						rem_sz;
 	sclp->address = page_address(kp->maplist[k]) + offset;
+#ifdef CONFIG_HIGHMEM
 	sclp->page = NULL;
+#endif
 	sclp->length = num;
 	mem_src_arr[k] = SG_USER_MEM;
 	rem_sz -= num;
@@ -1631,7 +1633,9 @@ static int sg_build_indi(Sg_scatter_hold
                     break;
             }
             sclp->address = p;
+#ifdef CONFIG_HIGHMEM
 	    sclp->page = NULL;
+#endif
             sclp->length = ret_sz;
 	    mem_src_arr[k] = mem_src;
 
@@ -1789,7 +1793,9 @@ static void sg_remove_scat(Sg_scatter_ho
                        k, sclp->address, sclp->length, mem_src));
             sg_free(sclp->address, sclp->length, mem_src);
             sclp->address = NULL;
+#ifdef CONFIG_HIGHMEM
 	    sclp->page = NULL;
+#endif
             sclp->length = 0;
         }
 	sg_free(schp->buffer, schp->sglist_len, schp->buffer_mem_src);
Index: drivers/scsi/st.c
===================================================================
RCS file: /var/cvs/linux/drivers/scsi/st.c,v
retrieving revision 1.10
diff -u -p -r1.10 st.c
--- drivers/scsi/st.c	2001/11/09 23:36:24	1.10
+++ drivers/scsi/st.c	2001/11/16 07:26:39
@@ -3233,7 +3233,9 @@ static ST_buffer *
 				break;
 			}
 		}
+#ifdef CONFIG_HIGHMEM
 		tb->sg[0].page = NULL;
+#endif
 		if (tb->sg[segs].address == NULL) {
 			kfree(tb);
 			tb = NULL;
@@ -3265,7 +3267,9 @@ static ST_buffer *
 					tb = NULL;
 					break;
 				}
+#ifdef CONFIG_HIGHMEM
 				tb->sg[segs].page = NULL;
+#endif
 				tb->sg[segs].length = b_size;
 				got += b_size;
 				segs++;
@@ -3339,7 +3343,9 @@ static int enlarge_buffer(ST_buffer * ST
 			normalize_buffer(STbuffer);
 			return FALSE;
 		}
+#ifdef CONFIG_HIGHMEM
 		STbuffer->sg[segs].page = NULL;
+#endif
 		STbuffer->sg[segs].length = b_size;
 		STbuffer->sg_segs += 1;
 		got += b_size;
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: parisc scatterlist doesn't want page/offset

[Jens Axboe]

On Fri, Nov 16 2001, Grant Grundler wrote:
> Hi all,
> Could someone point me to any discussion about adding
> page/offset to struct scatterlist?

See lkml a couple months back, 32-bit dma etc discussion.
> 
> To me, it looks like a half-assed step to support DMA to HIGHMEM
> on 32-bit arches.  TBH, I'd like to see page/offset replace
> address in the pci_map* interfaces and struct scatterlist.
> But then replace it across the board so the DMA mapping code
> doesn't have to decide which field to use (KISS). This really
> belongs in 2.5 kernel.

It's not half-assed, it's needed. I would imagine that pci_map_single
etc stays though, although pci_map_page or pci_map_sg is the preferred
approach.

Regarding the patch -- please add a helper function to set the sg list
instead of introducing CONFIG_HIGHMEM all over the place. I'm assuming
you are missing the piece which leaves out page/offset for non-highmem,
if not you are really pedantic about saving a few cycles :-)

-- 
Jens Axboe

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: parisc scatterlist doesn't want page/offset

[David S. Miller]

No, you must have page+offset because in the future the
"address" field of scatterlist is going to disappear
and _ONLY_ page+offset will be used.

Do not hack around it, just fix your scatterlist definition instead.
I'm trying to push a Documentation/DMA-mapping.txt update to Linus
which talks about this explicitly and requires your port to have those
fields. 
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: [parisc-linux] Re: parisc scatterlist doesn't want page/offset

[Matthew Wilcox]

On Fri, Nov 16, 2001 at 06:52:43AM -0800, David S. Miller wrote:
> 
> No, you must have page+offset because in the future the
> "address" field of scatterlist is going to disappear
> and _ONLY_ page+offset will be used.

but _WHY_ in 2.4?  this is ridiculous for something which is alleged to
be a stable kernel.

-- 
Revolutions do not require corporate support.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: [parisc-linux] Re: parisc scatterlist doesn't want page/offset

[David S. Miller]

   On Fri, Nov 16, 2001 at 06:52:43AM -0800, David S. Miller wrote:
   > 
   > No, you must have page+offset because in the future the
   > "address" field of scatterlist is going to disappear
   > and _ONLY_ page+offset will be used.
   
   but _WHY_ in 2.4?  this is ridiculous for something which is alleged to
   be a stable kernel.
   
You have to add two members to a silly structure which nobody
uses right now, that is so horrible.  What affect on stability
does that change have?

This makes merging of Jen's Axboe's block highmem code back into
2.4.x painless.  That is why.

Franks a lot,
David S. Miller
davem@redhat.com
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: [parisc-linux] Re: parisc scatterlist doesn't want page/offset

[Matthew Wilcox]

On Fri, Nov 16, 2001 at 07:17:51AM -0800, David S. Miller wrote:
>    From: Matthew Wilcox <willy@debian.org>
>    Date: Fri, 16 Nov 2001 15:04:54 +0000
> 
>    On Fri, Nov 16, 2001 at 06:52:43AM -0800, David S. Miller wrote:
>    > 
>    > No, you must have page+offset because in the future the
>    > "address" field of scatterlist is going to disappear
>    > and _ONLY_ page+offset will be used.
>    
>    but _WHY_ in 2.4?  this is ridiculous for something which is alleged to
>    be a stable kernel.
>    
> You have to add two members to a silly structure which nobody
> uses right now, that is so horrible.  What affect on stability
> does that change have?
> 
> This makes merging of Jen's Axboe's block highmem code back into
> 2.4.x painless.  That is why.

so when jens' code is merged back into 2.4 we won't have to make any
changes to the arch dependent code?

-- 
Revolutions do not require corporate support.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: [parisc-linux] Re: parisc scatterlist doesn't want page/offset

[David S. Miller]

   
   so when jens' code is merged back into 2.4 we won't have to make any
   changes to the arch dependent code?
   
Part of the criteria to whether we merge back Jens' code is
if the ports, given reasonable notice (ie. take this as your notice)
have added in the support for page+offset pairs to their pci_map_sg
code.

I suggest you do this now, it is totally painless.  I would almost
classify it as a mindless edit.

Franks a lot,
David S. Miller
davem@redhat.com
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: [parisc-linux] Re: parisc scatterlist doesn't want page/offset

[Matthew Wilcox]

On Fri, Nov 16, 2001 at 07:33:28AM -0800, David S. Miller wrote:
>    From: Matthew Wilcox <willy@debian.org>
>    Date: Fri, 16 Nov 2001 15:26:01 +0000
>    
>    so when jens' code is merged back into 2.4 we won't have to make any
>    changes to the arch dependent code?
>    
> Part of the criteria to whether we merge back Jens' code is
> if the ports, given reasonable notice (ie. take this as your notice)
> have added in the support for page+offset pairs to their pci_map_sg
> code.
> 
> I suggest you do this now, it is totally painless.  I would almost
> classify it as a mindless edit.

grant suggested adding support for it had performance implications,
so he wasn't willing to let me make that edit.  i'm not sure i entirely
understand the new scheme either.  the danger of updating code without
updating its supporting documentation.

-- 
Revolutions do not require corporate support.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: [parisc-linux] Re: parisc scatterlist doesn't want page/offset

[David S. Miller]

   
   grant suggested adding support for it had performance implications,
   so he wasn't willing to let me make that edit.  i'm not sure i entirely
   understand the new scheme either.  the danger of updating code without
   updating its supporting documentation.

If "address" is NULL, use page+offset.  It's pretty simple.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: [parisc-linux] Re: parisc scatterlist doesn't want page/offset

[Grant Grundler]

Matthew Wilcox wrote:
> grant suggested adding support for it had performance implications,

Jens is right to some degree - I'm counting cycles in that code
path since every driver depends on it for every IO request.
Until page/offset completely replaces address, I see a net increase
in cycles for parisc (probably most archs) with only a tangible benefit
for i386. Seems like the burden should be on the i386 folks to not
impact other arches.

My main gripe is adding this to 2.4 seems like the wrong time/place.
I'm happy to revisit this in 2.5.

grant

ps. I still have to go hunt down the discussion jens was referring to.
  Maybe if I understood the direction better, I'd be more receptive.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: [parisc-linux] Re: parisc scatterlist doesn't want page/offset

[Alan Cox]

> Until page/offset completely replaces address, I see a net increase
> in cycles for parisc (probably most archs) with only a tangible benefit
> for i386. Seems like the burden should be on the i386 folks to not
> impact other arches.
> My main gripe is adding this to 2.4 seems like the wrong time/place.
> I'm happy to revisit this in 2.5.

Its a bad impact on x86 so unfortunately it does need doing and I agree with
Dave - DaveM rarely implements something that is x86 beneficial but harms 
his beloved sparc64 without good reason.

Alan

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: [parisc-linux] Re: parisc scatterlist doesn't want page/offset

[Grant Grundler]

"David S. Miller" wrote:
> Part of the criteria to whether we merge back Jens' code is
> if the ports, given reasonable notice (ie. take this as your notice)
> have added in the support for page+offset pairs to their pci_map_sg
> code.

That is what willy was talking about.
You asking folks to muck with what's supposed to be working code.

> I suggest you do this now, it is totally painless.  I would almost
> classify it as a mindless edit.

Adding two members to a struct is not the problem.
The problem is revisiting every usage of ->address in the DMA code
and telling driver writers they should be using page+offset.

grant
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: [parisc-linux] Re: parisc scatterlist doesn't want page/offset

[David S. Miller]

   > I suggest you do this now, it is totally painless.  I would almost
   > classify it as a mindless edit.
   
   Adding two members to a struct is not the problem.
   The problem is revisiting every usage of ->address in the DMA code
   and telling driver writers they should be using page+offset.
   
Note the "should", nobody forces them to use page+offset in
a driver and 2.4.x will NEVER require it.  They just won't
be able to DMA highmem pages, that's all.

The DMA code is so simple to fix ("mindless edit" is still how I
classify it) and you have _THREE_ (count them, 3) platform IOMMU code
patch examples to work with (alpha, sparc64, ia64).

Franks a lot,
David S. Miller
davem@redhat.com
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: [parisc-linux] Re: parisc scatterlist doesn't want page/offset

[Grant Grundler]

"David S. Miller" wrote:
> Note the "should", nobody forces them to use page+offset in
> a driver and 2.4.x will NEVER require it.  They just won't
> be able to DMA highmem pages, that's all.

ok.  That's what I wanted to hear.

thanks,
grant
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/