From: Juergen Gross <jgross@suse.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Wei Liu <wei.liu@kernel.org>, Paul Durrant <paul@xen.org>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
xen-devel@lists.xenproject.org, linux-kernel@vger.kernel.org,
netdev@vger.kernel.org,
Ross Lagerwall <ross.lagerwall@citrix.com>
Subject: Re: [PATCH] xen/netback: fix build warning
Date: Wed, 7 Dec 2022 12:29:37 +0100 [thread overview]
Message-ID: <681773dd-6264-63ac-a3b5-a9182b9e0cc1@suse.com> (raw)
In-Reply-To: <0074a007-23a7-f2ff-0b85-47e4263c4d3f@suse.com>
[-- Attachment #1.1.1: Type: text/plain, Size: 3171 bytes --]
On 07.12.22 11:26, Jan Beulich wrote:
> On 07.12.2022 11:18, Juergen Gross wrote:
>> On 07.12.22 10:25, Jan Beulich wrote:
>>> On 07.12.2022 08:23, Juergen Gross wrote:
>>>> Commit ad7f402ae4f4 ("xen/netback: Ensure protocol headers don't fall in
>>>> the non-linear area") introduced a (valid) build warning.
>>>>
>>>> Fix it.
>>>>
>>>> Fixes: ad7f402ae4f4 ("xen/netback: Ensure protocol headers don't fall in the non-linear area")
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>
>>>> --- a/drivers/net/xen-netback/netback.c
>>>> +++ b/drivers/net/xen-netback/netback.c
>>>> @@ -530,7 +530,7 @@ static int xenvif_tx_check_gop(struct xenvif_queue *queue,
>>>> const bool sharedslot = nr_frags &&
>>>> frag_get_pending_idx(&shinfo->frags[0]) ==
>>>> copy_pending_idx(skb, copy_count(skb) - 1);
>>>> - int i, err;
>>>> + int i, err = 0;
>>>>
>>>> for (i = 0; i < copy_count(skb); i++) {
>>>> int newerr;
>>>
>>> I'm afraid other logic (below here) is now slightly wrong as well, in
>>> particular
>>>
>>> /* If the mapping of the first frag was OK, but
>>> * the header's copy failed, and they are
>>> * sharing a slot, send an error
>>> */
>>> if (i == 0 && !first_shinfo && sharedslot)
>>> xenvif_idx_release(queue, pending_idx,
>>> XEN_NETIF_RSP_ERROR);
>>> else
>>> xenvif_idx_release(queue, pending_idx,
>>> XEN_NETIF_RSP_OKAY);
>>>
>>> which looks to be intended to deal with _only_ failure of the one shared
>>> part of the header, whereas "err" now can indicate an error on any earlier
>>> part as well.
>>
>> The comment at the end of that loop seems to imply this is the desired
>> behavior:
>>
>> /* Remember the error: invalidate all subsequent fragments. */
>> err = newerr;
>> }
>
> This says "subsequent", whereas I was describing a situation where e.g.
> the first piece of header copying failed, the 2nd (shared part) succeeded,
> and the mapping of the rest of the shared part also succeeded. At the
> very least the comment in the code fragment I did quote then has become
> stale. Furthermore, if "all subsequent" really meant all, then in the
> new first loop this isn't followed either - an error response is sent
> only for the pieces where copying failed.
Having stared at the code for quite some time now, I think there is some
room for confusion: "invalidating" the frags seems not to be the same as
setting the related idx to have an error.
XEN_NETIF_RSP_ERROR seems to be set only for the idx which really had an
error, while if any of them had one, all idx-es must be unmapped, have a
status set, and returned to the frontend.
And I think the code is doing this quite fine.
The comments _could_ need some improvements, though.
And some more restructuring could help, too (at least I think that the
"goto check_frags" is a rather clumsy construct - I'd prefer splitting
xenvif_tx_check_gop() into some helper functions and a rather small
body calling those with e.g. different shinfo values).
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
next prev parent reply other threads:[~2022-12-07 11:29 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-07 7:23 [PATCH] xen/netback: fix build warning Juergen Gross
2022-12-07 9:25 ` Jan Beulich
2022-12-07 10:18 ` Juergen Gross
2022-12-07 10:26 ` Jan Beulich
2022-12-07 11:29 ` Juergen Gross [this message]
2022-12-07 12:24 ` Ross Lagerwall
2022-12-07 13:46 ` Jason Andryuk
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=681773dd-6264-63ac-a3b5-a9182b9e0cc1@suse.com \
--to=jgross@suse.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=jbeulich@suse.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=paul@xen.org \
--cc=ross.lagerwall@citrix.com \
--cc=wei.liu@kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
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;
as well as URLs for NNTP newsgroup(s).