On Friday 26 February 2010 03:46:45 pm Robert Hancock wrote:

On Fri, Feb 26, 2010 at 3:36 AM, David Miller [off-list ref] wrote:

quoted

From: Robert Hancock <redacted>
Date: Mon, 22 Feb 2010 20:45:45 -0600

quoted

Many networking drivers have issues with the use of the NETIF_F_HIGHDMA flag.
This flag actually indicates whether or not the device/driver can handle
skbs located in high memory (as opposed to lowmem). However, many drivers
incorrectly treat this flag as indicating that 64-bit DMA is supported, which
has nothing to do with its actual function. It makes no sense to make setting
NETIF_F_HIGHDMA conditional on whether a 64-bit DMA mask has been set, as many
drivers do, since if highmem DMA is supported at all, it should work regardless
of whether 64-bit DMA is supported. Failing to set NETIF_F_HIGHDMA when it
should be can hurt performance on architectures which use highmem since it
results in needless data copying.

This fixes up the networking drivers which currently use NETIF_F_HIGHDMA to
not do so conditionally on DMA mask settings.

For the USB kaweth and usbnet drivers, this patch also uncomments and corrects
some code to set NETIF_F_HIGHDMA based on the USB host controller's DMA mask.
These drivers should be able to access highmem unless the host controller is
non-DMA-capable, which is indicated by the DMA mask being null.

Signed-off-by: Robert Hancock <redacted>

Well, if the device isn't using 64-bit DMA addressing and the platform
uses direct (no-iommu) mapping of physical to DMA addresses , won't
your change break things?  The device will get a >4GB DMA address or
the DMA mapping layer will signal an error.

That's really part of the what the issue is I think.

So, this trigger the check in check_addr() in
arch/x86/kernel/pci-nommu.c when such packets try to get mapped by the
driver, right?

That will make the DMA mapping call fail, and the packet will be
dropped permanently.  And hey, on top of it, many of these drivers you
remove the setting from don't even check the mapping call return
values for errors.

So even bigger breakage.  One example is drivers/net/8139cp.c,
it just does dma_map_single() and uses the result.

It really depends upon that NETIF_F_HIGHDMA setting for correct
operation.

And even if something like swiotlb is available, now we're going
to do bounce buffering which is largely equivalent to what
a lack of NETIF_F_HIGHDMA will do.  Except that once NETIF_F_HIGHDMA
copies the packet to lowmem it will only do that once, whereas if
the packet goes to multiple devices swiotlb might copy the packet
to a bounce buffer multiple times.

We definitely can't apply your patch as-is.

Hmm.. Yeah, there is a bit of a mess there. I'm thinking of the
particular example of i386 where you have 32-bit DMA devices with more
than 4GB of RAM. If you then allow the device to access highmem then
the DMA mapping API can get presented with addresses above 4GB and
AFAIK I don't think it can cope with that situation on that platform.

Problem is that the NETIF_F_HIGHDMA check is generally too restrictive
in that situation, and it's really conflating two things into one (the
genuine can't-access-highmem part, and the "oh by the way, if highmem
can be >4GB then we can't access that") . If you have 3GB of RAM on
i386 with one of these drivers, you'll have packets being bounced
through lowmem without any real reason. I'll have a look into things a
bit further..

Maybe it would be useful to start with splitting NETIF_F_HIGHDMA on two
independent flags, i.e.:

	#define	NETIF_F_DMA_HIGH	(1 << 27)
	#define NETIF_F_DMA_64BIT	(1 << 28)

and keeping NETIF_F_HIGHDMA as

	#define NETIF_F_HIGHDMA		(NETIF_F_DMA_HIGH | NET_F_DMA_64BIT)

for now..?

[ Next step would involve fixing illegal_highdma() check in net/core/dev.c
  to distinguish between those new flags which in turn should allow sorting
  out code in the device drivers on *per-driver* basis. ]

--
Bartlomiej Zolnierkiewicz