Re: [PATCH] copy_from_high_bh

Re: [PATCH] copy_from_high_bh

[Mark Hemment]

On Sun, 8 Jul 2001, Linus Torvalds wrote:
> On Sun, 8 Jul 2001 markhe@veritas.com wrote:
> >
> >   mm/highmem.c/copy_from_high_bh() blocks interrupts while copying "down"
> > to a bounce buffer, for writing.
> >   This function is only ever called from create_bounce() (which cannot
> > be called from an interrupt context - it may block), and the kmap
> > 'KM_BOUNCE_WRITE' is only used in copy_from_high_bh().
>
> If so it's not just the interrupt disable that is unnecessary, the
> "kmap_atomic()" should also be just a plain "kmap()", I think.

  That might eat through kmap()'s virtual window too quickly.

  I did think about adding a test to see if the page was already mapped,
and falling back to kmap_atomic() if it isn't.  That should give the best
of both worlds?

Mark

Re: [PATCH] copy_from_high_bh

[Andrea Arcangeli]

On Sun, Jul 08, 2001 at 07:26:01PM +0100, Mark Hemment wrote:
> On Sun, 8 Jul 2001, Linus Torvalds wrote:
> > On Sun, 8 Jul 2001 markhe@veritas.com wrote:
> > >
> > >   mm/highmem.c/copy_from_high_bh() blocks interrupts while copying "down"
> > > to a bounce buffer, for writing.
> > >   This function is only ever called from create_bounce() (which cannot
> > > be called from an interrupt context - it may block), and the kmap
> > > 'KM_BOUNCE_WRITE' is only used in copy_from_high_bh().
> >
> > If so it's not just the interrupt disable that is unnecessary, the
> > "kmap_atomic()" should also be just a plain "kmap()", I think.
> 
>   That might eat through kmap()'s virtual window too quickly.

certainly not more quickly than a flood of allocation of highmem pages
in anon mem. I don't see your point. kmap has to work just fine over
there and the __cli/__sti was obviously useless. (should be faster
because we won't need to invlpg at every bounce, but OTOH we'll flush
the whole tlb after the wrap around but as said above if flushing the
tlb too frequently is the issue we should simply improve kmap because
the flood of kmap will happen anyways in the very fast paths regardless
of the bounces)

>   I did think about adding a test to see if the page was already mapped,
> and falling back to kmap_atomic() if it isn't.  That should give the best
> of both worlds?
> 
> Mark


Andrea

[PATCH] copy_from_high_bh

[markhe]

Hi,

  mm/highmem.c/copy_from_high_bh() blocks interrupts while copying "down"
to a bounce buffer, for writing.
  This function is only ever called from create_bounce() (which cannot
be called from an interrupt context - it may block), and the kmap
'KM_BOUNCE_WRITE' is only used in copy_from_high_bh().

  Therefore, the comment in copy_from_high_bh() is wrong, and the
__cli()/__sti() unnecessary.

  Patch against 2.4.6 below.

Mark

-------------------------------------------------------------------

diff -urN -X dontdiff linux-2.4.6/mm/highmem.c cli-2.4.6/mm/highmem.c
--- linux-2.4.6/mm/highmem.c	Fri Jun 29 23:17:34 2001
+++ cli-2.4.6/mm/highmem.c	Sun Jul  8 15:50:04 2001
@@ -182,20 +182,12 @@
 {
 	struct page *p_from;
 	char *vfrom;
-	unsigned long flags;

 	p_from = from->b_page;

-	/*
-	 * Since this can be executed from IRQ context, reentrance
-	 * on the same CPU must be avoided:
-	 */
-	__save_flags(flags);
-	__cli();
 	vfrom = kmap_atomic(p_from, KM_BOUNCE_WRITE);
 	memcpy(to->b_data, vfrom + bh_offset(from), to->b_size);
 	kunmap_atomic(vfrom, KM_BOUNCE_WRITE);
-	__restore_flags(flags);
 }

 static inline void copy_to_high_bh_irq (struct buffer_head *to,