From: Ezequiel Garcia <ezequiel.garcia@free-electrons•com>
To: Russell King - ARM Linux <linux@arm•linux.org.uk>
Cc: netdev@vger•kernel.org, David Miller <davem@davemloft•net>,
B38611@freescale•com, fabio.estevam@freescale•com
Subject: Re: [PATCH 2/2] net: mv643xx_eth: Fix highmem support in non-TSO egress path
Date: Thu, 22 Jan 2015 09:17:18 -0300 [thread overview]
Message-ID: <54C0EA4E.2070208@free-electrons.com> (raw)
In-Reply-To: <20150122001103.GY26493@n2100.arm.linux.org.uk>
On 01/21/2015 09:11 PM, Russell King - ARM Linux wrote:
> On Wed, Jan 21, 2015 at 08:34:30PM -0300, Ezequiel Garcia wrote:
>> I have just realised that the non-TSO and the TSO paths must work
>> simultaneously (we don't know which path an egress skb will take).
>>
>> So, with these patches, the unmapping is done using dma_unmap_page() which
>> is only correct if the skb took the non-TSO paths. In other words,
>> these fixes are wrong (although I have no idea the effect of
>> using dma_unmap_page on a mapping done with dma_map_single).
>>
>> And the problem is that in the TSO path, the linear and the non-linear
>> fragments use the same kind of descriptors, so we can't distinguish
>> them in the cleanup, and can't decide if _single or _page should be used.
>>
>> Any ideas?
>
> Or, maybe, if davem would reply, we might come to the conclusion (as
> I previously pointed out) that it's not a driver issue, but a netdev
> core issue:
>
> static netdev_features_t harmonize_features(struct sk_buff *skb,
> netdev_features_t features)
> {
> ...
> if (skb->ip_summed != CHECKSUM_NONE &&
> !can_checksum_protocol(features, type)) {
> features &= ~NETIF_F_ALL_CSUM;
> } else if (illegal_highdma(skb->dev, skb)) {
> features &= ~NETIF_F_SG;
> }
>
> The problem is when the first "if" is true (as is the case with IPv6 on
> mv643xx_eth.c), we clear NETIF_F_ALL_CSUM, but leave NETIF_F_SG set.
>
> Had that first if been false, we would've called illegal_highdma(), and
> found that the skb contains some highmem fragments, but the device does
> *not* have NETIF_F_HIGHDMA set, and so that second "if" would be true.
> The result of that is NETIF_F_SG is cleared.
>
> In this case, in validate_xmit_skb(), skb_needs_linearize() would be
> false for a skb with fragments, causing the skb to be linearised. I've
> not completely traced the GSO path, but I'd assume that does something
> similar (which I think skb_segment() handles.)
>
> So, I'm wondering whether the above should be:
>
> static netdev_features_t harmonize_features(struct sk_buff *skb,
> netdev_features_t features)
> {
> ...
> if (skb->ip_summed != CHECKSUM_NONE &&
> !can_checksum_protocol(features, type)) {
> features &= ~NETIF_F_ALL_CSUM;
> }
>
> if (illegal_highdma(skb->dev, skb)) {
> features &= ~NETIF_F_SG;
> }
>
> So that we get NETIF_F_SG turned off for all cases (irrespective of the
> NETIF_F_ALL_CSUM test) if we see a skb with highmem and we the device
> does not support highdma.
>
> Yes, the code above hasn't changed in functionality for a long time, but
> that doesn't mean it isn't buggy, and isn't the cause of our current bug.
>
Hm, that's interesting.
> However, it would be far better to have the drivers fixed for the sake
> of performance - it's only this dma_map_page() thing that is the real
> cause of the problem in these drivers.
>
Yes, I have just sent a v2 to fix the mv643xx_eth driver (non-TSO path).
If that works, I'll see about preparing a fix for mvneta, and for both
egress paths.
> Looking at TSO, it seems madness that it doesn't support highmem:
>
> void tso_start(struct sk_buff *skb, struct tso_t *tso)
> {
> ...
> tso->data = skb->data + hdr_len;
> ...
> tso->data = page_address(frag->page.p) + frag->page_offset;
>
> Of course, this would all be a lot easier for drivers if all drivers had
> to worry about was a struct page, offset and size, rather than having to
> track whether each individual mapping of a transmit packet was mapped
> with dma_map_single() or dma_map_page().
>
> That all said, what I really care about is the regression which basically
> makes 3.18 unusable on this hardware and seeing _some_ kind of resolution
> to that regression - I don't care if it doesn't quite perform, what I care
> about is that the network driver doesn't oops the kernel.
>
Thanks for all the info!
--
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
next prev parent reply other threads:[~2015-01-22 12:19 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-01-21 12:54 [PATCH net 0/2] net: marvell: Fix highmem support on non-TSO path Ezequiel Garcia
2015-01-21 12:54 ` [PATCH 1/2] net: mvneta: Fix highmem support in the non-TSO egress path Ezequiel Garcia
2015-01-26 22:40 ` David Miller
2015-01-21 12:54 ` [PATCH 2/2] net: mv643xx_eth: Fix highmem support in " Ezequiel Garcia
2015-01-21 17:40 ` Russell King - ARM Linux
2015-01-21 23:34 ` Ezequiel Garcia
2015-01-22 0:11 ` Russell King - ARM Linux
2015-01-22 12:17 ` Ezequiel Garcia [this message]
2015-01-26 22:40 ` David Miller
2015-01-21 15:01 ` [PATCH net 0/2] net: marvell: Fix highmem support on non-TSO path Russell King - ARM Linux
2015-01-22 18:41 ` Dean Gehnert
2015-01-22 18:45 ` Ezequiel Garcia
2015-01-22 19:01 ` Dean Gehnert
2015-01-22 21:09 ` Russell King - ARM Linux
2015-01-22 21:27 ` Dean Gehnert
2015-01-22 21:49 ` Russell King - ARM Linux
2015-01-22 23:06 ` Russell King - ARM Linux
2015-01-22 23:09 ` Dean Gehnert
2015-01-22 23:08 ` Dean Gehnert
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=54C0EA4E.2070208@free-electrons.com \
--to=ezequiel.garcia@free-electrons$(echo .)com \
--cc=B38611@freescale$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=fabio.estevam@freescale$(echo .)com \
--cc=linux@arm$(echo .)linux.org.uk \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox