From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_PASS, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 99927C43387 for ; Thu, 17 Jan 2019 15:17:03 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 54B47205C9 for ; Thu, 17 Jan 2019 15:17:03 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="ifCxmR9I" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727487AbfAQPRC (ORCPT ); Thu, 17 Jan 2019 10:17:02 -0500 Received: from mail-ed1-f65.google.com ([209.85.208.65]:47021 "EHLO mail-ed1-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725882AbfAQPRB (ORCPT ); Thu, 17 Jan 2019 10:17:01 -0500 Received: by mail-ed1-f65.google.com with SMTP id o10so8593243edt.13 for ; Thu, 17 Jan 2019 07:17:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=eRxJLdDSO3B25eElT5cEcowwJgyCaxoMWFTXtVV4Yig=; b=ifCxmR9Ix07JoOt4ZcIswAH/mJnBb/ItLpXcemghP9/JqVpNbha5tbSCF63QZADH7Y CasSfphTYQ6oYKkT8qcq1PFxdFegeC4Tg8RMqj3LswVEPrYU/3vxrbQ6M1MtOqpwPH42 tHD/uep3WKpZJracCZtkkMhvISWWJ2TmrCm9/SsgWkS7gqaIGbJiGPyvba8QtgpjRadD 2aEgAOItDDa+yBLOyhcLOb9DJPE9YJ+pQwLZe6uunuT0pnzRCfj0zmqi/R6anZpbQQQG UPb9HApg/LFCTHyBUILjSgTOXFX4CYou5Xr+PdSrhZVCp5joJFc6YSkjWjKZRTWMI9Rz RRKA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=eRxJLdDSO3B25eElT5cEcowwJgyCaxoMWFTXtVV4Yig=; b=rjrNwpOTPHwxmJBg3qZS4Vf9g+Pca3eFf/b876MDJaV8bNVzl3A9o+vfhzglcWZ4DC bFPcUDJvUdtlPrjgS8t3cK51L+kovebefbqGF05X9emc+M/XFeMuO5pxBXT757miF1ZK KRi4e++DKA7Uq0UVsp67tW96LBX8IMd87x5p9NzOVsRmAEjprydUa5Ss1QtrcqTlX569 BBPdZPL3KO0L1EZyYztdA/FjsBCliFDpnfpc7aDl1T2rmyW/OB3qHA8tFEQPhAMHMycL kuSvBsLy+nxQdOaT7qQm0AvLcDaKRaXBYAg+PWcnGtuPtHUp9lbi446b+fNeksqLVM8i 2KGw== X-Gm-Message-State: AJcUukfaPprLZT0MxizdT2kmisT/6EMzJlB715LvN9BAvXZwmKpi8Zp6 EUv+J05RL8sBTh4Diq0ShMqSv4DLT4h+QTyUyDc= X-Google-Smtp-Source: ALg8bN5JSM/X/S7tjV6EzGeDxUycbpvYGExgGtaEux39nb5vGpgtmZLZoLEalVHx5UvwPVCcanYpCa2D5/F6jdgcpLo= X-Received: by 2002:a17:906:61cc:: with SMTP id t12-v6mr10655520ejl.181.1547738219767; Thu, 17 Jan 2019 07:16:59 -0800 (PST) MIME-Version: 1.0 References: <20190114131841.1932-1-maximmi@mellanox.com> <20190114131841.1932-2-maximmi@mellanox.com> In-Reply-To: From: Willem de Bruijn Date: Thu, 17 Jan 2019 10:16:23 -0500 Message-ID: Subject: Re: [PATCH 1/7] net: Don't set transport offset to invalid value To: Maxim Mikityanskiy Cc: "David S. Miller" , Saeed Mahameed , Willem de Bruijn , Jason Wang , Eric Dumazet , "netdev@vger.kernel.org" , Eran Ben Elisha , Tariq Toukan Content-Type: text/plain; charset="UTF-8" Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Thu, Jan 17, 2019 at 4:10 AM Maxim Mikityanskiy wrote: > > > This is a lot of code change. This would do. > > > > @@ -2434,8 +2434,6 @@ static inline void > > skb_probe_transport_header(struct sk_buff *skb, > > > > if (skb_flow_dissect_flow_keys_basic(skb, &keys, NULL, 0, 0, 0, 0)) > > skb_set_transport_header(skb, keys.control.thoff); > > - else > > - skb_set_transport_header(skb, offset_hint); > > } > > > > Though leaving an unused argument is a bit ugly. For net-next, indeed > > better to clean up (please mark your patchset with net or net-next, > > btw) > > It's for net-next (I'll resend with the correct mark), so I'll stick > with the current implementation. Absolutely, sounds good. > > > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen- > > netback/netback.c > > > index 80aae3a32c2a..b49b6e56ca47 100644 > > > --- a/drivers/net/xen-netback/netback.c > > > +++ b/drivers/net/xen-netback/netback.c > > > @@ -1105,6 +1105,7 @@ static int xenvif_tx_submit(struct xenvif_queue > > *queue) > > > struct xen_netif_tx_request *txp; > > > u16 pending_idx; > > > unsigned data_len; > > > + bool th_set; > > > > > > pending_idx = XENVIF_TX_CB(skb)->pending_idx; > > > txp = &queue->pending_tx_info[pending_idx].req; > > > @@ -1169,20 +1170,22 @@ static int xenvif_tx_submit(struct xenvif_queue > > *queue) > > > continue; > > > } > > > > > > - skb_probe_transport_header(skb, 0); > > > + th_set = skb_try_probe_transport_header(skb); > > > > Can use skb_transport_header_was_set(). Then at least there is no need > > to change the function's return value. > > I suppose this comment relates to the previous one, and if we do it for > net-next, it's fine to make change I made, isn't it? If this is the only reason for the boolean return value, using skb_transport_header_was_set() is more standard (I immediately know what's happening when I read it), slightly less code change and avoids introducing a situation where the majority of callers ignore a return value. I think it's preferable. But these merits are certainly debatable, so either is fine.