* [PATCH] xen/netback: fix build warning @ 2022-12-07 7:23 Juergen Gross 2022-12-07 9:25 ` Jan Beulich ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Juergen Gross @ 2022-12-07 7:23 UTC (permalink / raw) To: linux-kernel, netdev Cc: Juergen Gross, Wei Liu, Paul Durrant, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, xen-devel 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> --- drivers/net/xen-netback/netback.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c index 054ac0e897f6..bf627af723bf 100644 --- 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; -- 2.35.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] xen/netback: fix build warning 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 12:24 ` Ross Lagerwall 2022-12-07 13:46 ` Jason Andryuk 2 siblings, 1 reply; 7+ messages in thread From: Jan Beulich @ 2022-12-07 9:25 UTC (permalink / raw) To: Juergen Gross Cc: Wei Liu, Paul Durrant, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, xen-devel, linux-kernel, netdev, Ross Lagerwall 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. Jan ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xen/netback: fix build warning 2022-12-07 9:25 ` Jan Beulich @ 2022-12-07 10:18 ` Juergen Gross 2022-12-07 10:26 ` Jan Beulich 0 siblings, 1 reply; 7+ messages in thread From: Juergen Gross @ 2022-12-07 10:18 UTC (permalink / raw) To: Jan Beulich Cc: Wei Liu, Paul Durrant, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, xen-devel, linux-kernel, netdev, Ross Lagerwall [-- Attachment #1.1.1: Type: text/plain, Size: 1727 bytes --] 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; } 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xen/netback: fix build warning 2022-12-07 10:18 ` Juergen Gross @ 2022-12-07 10:26 ` Jan Beulich 2022-12-07 11:29 ` Juergen Gross 0 siblings, 1 reply; 7+ messages in thread From: Jan Beulich @ 2022-12-07 10:26 UTC (permalink / raw) To: Juergen Gross Cc: Wei Liu, Paul Durrant, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, xen-devel, linux-kernel, netdev, Ross Lagerwall 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. Jan ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xen/netback: fix build warning 2022-12-07 10:26 ` Jan Beulich @ 2022-12-07 11:29 ` Juergen Gross 0 siblings, 0 replies; 7+ messages in thread From: Juergen Gross @ 2022-12-07 11:29 UTC (permalink / raw) To: Jan Beulich Cc: Wei Liu, Paul Durrant, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, xen-devel, linux-kernel, netdev, Ross Lagerwall [-- 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xen/netback: fix build warning 2022-12-07 7:23 [PATCH] xen/netback: fix build warning Juergen Gross 2022-12-07 9:25 ` Jan Beulich @ 2022-12-07 12:24 ` Ross Lagerwall 2022-12-07 13:46 ` Jason Andryuk 2 siblings, 0 replies; 7+ messages in thread From: Ross Lagerwall @ 2022-12-07 12:24 UTC (permalink / raw) To: Juergen Gross, linux-kernel, netdev Cc: Wei Liu, Paul Durrant, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, xen-devel > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> on behalf of Juergen Gross <jgross@suse.com> > Sent: Wednesday, December 7, 2022 7:23 AM > To: linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; netdev@vger.kernel.org <netdev@vger.kernel.org> > Cc: Juergen Gross <jgross@suse.com>; 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 <xen-devel@lists.xenproject.org> > Subject: [PATCH] xen/netback: fix build warning > > 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: Ross Lagerwall <ross.lagerwall@citrix.com> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xen/netback: fix build warning 2022-12-07 7:23 [PATCH] xen/netback: fix build warning Juergen Gross 2022-12-07 9:25 ` Jan Beulich 2022-12-07 12:24 ` Ross Lagerwall @ 2022-12-07 13:46 ` Jason Andryuk 2 siblings, 0 replies; 7+ messages in thread From: Jason Andryuk @ 2022-12-07 13:46 UTC (permalink / raw) To: Juergen Gross Cc: linux-kernel, netdev, Wei Liu, Paul Durrant, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, xen-devel On Wed, Dec 7, 2022 at 2:24 AM Juergen Gross <jgross@suse.com> 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> Tested-by: Jason Andryuk <jandryuk@gmail.com> I applied ad7f402ae4f4 to 5.15.y and 5.4.y and it broke networking with my driver domains. The frontend failed to DHCP an address and it didn't look like any packets were getting through. This patch fixed networking with 5.15.y and 5.4.y. I think the commit message is worth expanding that this is more than just a build warning. Thanks, Jason ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-12-07 13:46 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 2022-12-07 12:24 ` Ross Lagerwall 2022-12-07 13:46 ` Jason Andryuk
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).