From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-f195.google.com (mail-pg1-f195.google.com [209.85.215.195]) by dpdk.org (Postfix) with ESMTP id E208D5B3A for ; Thu, 24 Jan 2019 19:05:43 +0100 (CET) Received: by mail-pg1-f195.google.com with SMTP id v28so2981505pgk.10 for ; Thu, 24 Jan 2019 10:05:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nfware-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=PZFbUkKbyuVyQabOMipn66uyz6e5v4V4Fb+wMbmUd8E=; b=lHPZRKyKI6fRoApDuKwGvOz4h9D6/drOfcGy3fky5Xt1UZvrW4/SEUxtMMG0VkUrRv AHaQWJ1iyhWvQgK579SPIVMSZ+3lvMkZUOXbkEFGrHKZ5TQXZIC7o/pc+ODL6asppUqd wTjLFZAORfAhtjDLRxCKPkwyqUoEL5qwAr68i8Lm+wKFMfYszpx6NKV4TcdDV57CcBC8 9yiZ1FR0Uz48P82lx8fFv6D4dJl4o+WW1RSSICCpfD5G4WZe2JQVFz/ffUIA9xw9S53/ J4kt6fX4gI8fDUgzYoN5saQInOEwcRw5YDm3v8Ga5tBRErKdrGDWvpqCG/KSpyU9ruKx aajQ== 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=PZFbUkKbyuVyQabOMipn66uyz6e5v4V4Fb+wMbmUd8E=; b=iYiCSpKv7KhNPwGw0kqdfZt2cM64TeQQihpnLhcOW0w4dWGBoezU1Tq1TVYsNVolMy IVgSqfN8NoDW3z9vAEtEgijCT7ZSsgU0L6NcjXR587ze6IvEj5Aw3xpZ2Yua/V8JndBN FStMGK6AH1DXPgenfx+gVJQ+YloGmV7RkSCZLraYRHNIsCGOrSw4FZ32jRgH8qT/DnXY FOlX7nErpQ777fJYJWvMMEcih1WcBZmMqeIxtXQ/1vZVBEf73bF7eZCmIiPg99FEv7+1 9w9mseSkWMYQqL6Uw+CAuvyWxgSEfGLvTODK+DVG3i6ifGo9da2dwYIgRqyrGkw+V5CN evvQ== X-Gm-Message-State: AJcUukfmsZK/zrrPxKtHBCDSFEWPw/Am1q9uMDbgvgPUPC1kQ/7y6O6X oYKw/ZUMYBykJPY2lDw7YQN8qFQonuB+84kwwBR4GQ== X-Google-Smtp-Source: ALg8bN7INE5WFB/7bKqiObZHMlAg8ERbeWn3JXtiWuw6YGDTm47Wg+QDL/mkSAgLqpkEOBGbCYTP7hWiTAOK8wqchJ8= X-Received: by 2002:a63:ab08:: with SMTP id p8mr6727496pgf.87.1548353142924; Thu, 24 Jan 2019 10:05:42 -0800 (PST) MIME-Version: 1.0 References: <20180927000224.4011-1-iryzhov@nfware.com> <0a5223b8-5b32-966a-7339-617957c7ba45@intel.com> In-Reply-To: From: Igor Ryzhov Date: Thu, 24 Jan 2019 21:05:31 +0300 Message-ID: To: Ferruh Yigit Cc: dev@dpdk.org, Stephen Hemminger Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH] kni: implement header_ops parse method X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 24 Jan 2019 18:05:44 -0000 Hi Ferruh, Ok, no problem. Generally, it is needed for all applications using packet(7) interface, running over KNI interfaces. More specifically, one example of such application is FRRouting, I suppose you are familiar with it. FRR's ISIS daemon is using AF_PACKET sockets and checking received sockaddr_ll. Here is the link on one of the usages =E2=80=93 https://github.com/FRRouting/frr/blob/master/isisd/isis_pfpacket.c#L294 When we are good with motivation, I'll send v2 using eth_header_parse. Best regards, Igor On Thu, Jan 24, 2019 at 8:15 PM Ferruh Yigit wrote= : > On 1/24/2019 4:35 PM, Igor Ryzhov wrote: > > Hi Ferruh, > > > > I already answered your question in my previous email, header_ops.parse > method > > is used by packet(7) interface for packet parsing and filling > sockaddr_ll structure. > > Here is the link on the usage =E2=80=93 > > > https://elixir.bootlin.com/linux/latest/source/net/packet/af_packet.c#L21= 00 > > I saw your previous reply. That is why changed the question slightly, fro= m > 'what > it does' to 'what is the use case'. > Trying to understand do we need it, please help me understand. > > > > > Regarding the question about eth_header_ops usage: > > Right now both already existing functions, kni_net_header and > > kni_net_rebuild_header, are implemented as copies, not using > eth_header_ops > > functions. > > OK, I see your reasoning, but if there is an already Linux API that does > what we > want, I suggest calling it instead of re-implementing it, unless there is > a good > reason to not do so. > > > That's why I think my patch should be accepted as is, and the problem o= f > > eth_header_ops usage should be investigated separately, and possibly > resolved by > > a separate patch. > > Agreed, eth_header_ops usage should be investigated separately. > > > > > Best regards, > > Igor > > > > On Thu, Jan 24, 2019 at 5:10 PM Ferruh Yigit > > wrote: > > > > On 1/24/2019 9:18 AM, Igor Ryzhov wrote: > > > Hi Ferruh, > > > > > > What about this patch? > > > Can you merge it as-is, or should I change it to use relevant > eth_header_ops > > > functions? Or maybe completely use eth_header_ops? > > > > Hi Igor, > > > > I am not clear about motivation of the patch, what use case enabled > by this > > patch? What is not working with current code? > > I am for rejecting the patch without need justified. > > > > And if the need is justified, still there is a question that why no= t > use > > 'eth_header_parse()' directly but implement our copy? > > > > > > And an extended question/investigation about why not use > 'eth_header_ops', which > > seems done intentionally but I am missing the reasoning. > > > > > > > > Best regards, > > > Igor > > > > > > On Fri, Nov 30, 2018 at 10:07 PM Igor Ryzhov > > > > >> wrote: > > > > > > Hi Ferruh, > > > > > > header_ops.parse method is used by raw-sockets to > fill sockaddr_ll > > structure. > > > It is used, for example, in isisd for frrouting. > > > > > > Regarding your question about eth_header_ops =E2=80=93 I, > unfortunately, don't > > know > > > why .cache and .cache_update are disabled for KNI. > > > I also think that it will be better to use default > eth_header_ops. > > > > > > Best regards, > > > Igor > > > > > > On Tue, Oct 2, 2018 at 7:58 PM Ferruh Yigit < > ferruh.yigit@intel.com > > > > > >> > wrote: > > > > > > On 9/27/2018 1:02 AM, Igor Ryzhov wrote: > > > > Signed-off-by: Igor Ryzhov > > > > >> > > > > > > Hi Igor, > > > > > > What is the motivation to add this support? What is > enabled by this? > > > > > > > > > Meanwhile, why we are not using eth_header_ops, which is > already > > set by > > > ether_setup(). > > > To disable .cache & .cache_update? > > > > > > If so why not using relevant eth_header_ops (eth_header, > > > eth_header_parse ..) > > > instead of implementing ours? > > > > > > > --- > > > > kernel/linux/kni/kni_net.c | 14 ++++++++++++++ > > > > 1 file changed, 14 insertions(+) > > > > > > > > diff --git a/kernel/linux/kni/kni_net.c > b/kernel/linux/kni/kni_net.c > > > > index 7fcfa106c..128a5477c 100644 > > > > --- a/kernel/linux/kni/kni_net.c > > > > +++ b/kernel/linux/kni/kni_net.c > > > > @@ -678,6 +678,19 @@ kni_net_header(struct sk_buff *skb= , > struct > > > net_device *dev, > > > > return dev->hard_header_len; > > > > } > > > > > > > > +/* > > > > + * Extract hardware address from packet > > > > + */ > > > > +static int > > > > +kni_net_header_parse(const struct sk_buff *skb, > unsigned char > > *haddr) > > > > +{ > > > > + const struct ethhdr *eth =3D eth_hdr(skb); > > > > + > > > > + memcpy(haddr, eth->h_source, ETH_ALEN); > > > > + > > > > + return ETH_ALEN; > > > > +} > > > > + > > > > /* > > > > * Re-fill the eth header > > > > */ > > > > @@ -739,6 +752,7 @@ kni_net_change_carrier(struct > net_device *dev, > > > bool new_carrier) > > > > > > > > static const struct header_ops kni_net_header_ops =3D = { > > > > .create =3D kni_net_header, > > > > + .parse =3D kni_net_header_parse, > > > > #ifdef HAVE_REBUILD_HEADER > > > > .rebuild =3D kni_net_rebuild_header, > > > > #endif /* < 4.1.0 */ > > > > > > > > > > >