From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-f194.google.com (mail-pf1-f194.google.com [209.85.210.194]) by dpdk.org (Postfix) with ESMTP id 2E3254CA9 for ; Mon, 8 Apr 2019 18:59:35 +0200 (CEST) Received: by mail-pf1-f194.google.com with SMTP id t21so5657103pfh.2 for ; Mon, 08 Apr 2019 09:59:35 -0700 (PDT) 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=KT4UMEDMfBliZq1v5DrhixRvNz8VWK1UssCvJfXzF+A=; b=1sGmogTQ6DS60rW6XQOZOtHirUktMi9Zz40LI96Vy8NlY7y7m3GlnmElKT0my5+tBK lCb8zUdv+GjigiWDs5g4d0DSxsw7OWBOTVWhwHr3hHHz35dXeZwxvoSWveZ7bkRvQLL4 au4SZ0MLch5ergwV2vN5gBWAFCLIbhsvXKg1j9K3pt+CoQv9dcAzCt2KJdHQWO/HZ4Qu /zEpZkKiLfuso73nRfjwdqjduc/hyXPL7aF8m0so7Q6kjFfMqLoY4btsBLOj7cD1sBCj 231xcCmoR5MSs35HjXmqst525hGXUWHW7pb8hr2pg519NK3Hv7u2vqrB/Ig3AXr5V2+Z DzFw== 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=KT4UMEDMfBliZq1v5DrhixRvNz8VWK1UssCvJfXzF+A=; b=uQi1Cax9Pg7Mi9RmxlOuI5GSeva5/WFd6pQVJsHDo+paR++V0i/bQxFjfreXj+KRzA a/PxieyC+KEKY3q14fZseF+IhUCiMEOjdxJsg8CfIrVgMimRDtAyl397I8DShk/6AACB obRS1LJXtjwjKW+1ETXGj3j6ztSoESg1BOG53NKeLRWTCkBQu1D2yh4KrdhaeuZPhWO+ LS8lfpESNmgg742tdMahjAYJBxN0Cg7Mf34EgYbH9dcEuYF5uZ64CHXx+iaEOuAZnUQL nnEM4skaa2U3M4ml+pky4xjf7IC50Cd6fLYQ903ZRqLx5JFc68YjIYhsnHQHcbENTIGK kujw== X-Gm-Message-State: APjAAAXP853d93TAaOSJeTPfWb5dR/GSjK1aiXqcqBPbZ+jS8yJ1c3tO k9Z+3VGSGfbiq/Lo8XpEShR9A9wyFy/imgjs95yoVQ== X-Google-Smtp-Source: APXvYqzbAMgEOdphgOUBwsn2wr1nNBKvv61PpLN8RqYqkcqXdVtaFoGwmF9EtbTYhsTyBSawiMI4wbRpNtUQ6lZTIlA= X-Received: by 2002:aa7:8518:: with SMTP id v24mr31090291pfn.219.1554742774344; Mon, 08 Apr 2019 09:59:34 -0700 (PDT) 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: Mon, 8 Apr 2019 19:59:23 +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: Mon, 08 Apr 2019 16:59:35 -0000 Hi Ferruh, Can we proceed with this patch? Igor On Thu, Jan 24, 2019 at 9:05 PM Igor Ryzhov wrote: > 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 suppos= e > 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.pars= e >> 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#L2= 100 >> >> I saw your previous reply. That is why changed the question slightly, >> from '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 i= s >> a good >> reason to not do so. >> >> > That's why I think my patch should be accepted as is, and the problem = of >> > 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 enable= d >> 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 >> not 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 */ >> > > > >> > > >> > >> >> From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id 24D85A0096 for ; Mon, 8 Apr 2019 18:59:37 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id C73184D3A; Mon, 8 Apr 2019 18:59:36 +0200 (CEST) Received: from mail-pf1-f194.google.com (mail-pf1-f194.google.com [209.85.210.194]) by dpdk.org (Postfix) with ESMTP id 2E3254CA9 for ; Mon, 8 Apr 2019 18:59:35 +0200 (CEST) Received: by mail-pf1-f194.google.com with SMTP id t21so5657103pfh.2 for ; Mon, 08 Apr 2019 09:59:35 -0700 (PDT) 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=KT4UMEDMfBliZq1v5DrhixRvNz8VWK1UssCvJfXzF+A=; b=1sGmogTQ6DS60rW6XQOZOtHirUktMi9Zz40LI96Vy8NlY7y7m3GlnmElKT0my5+tBK lCb8zUdv+GjigiWDs5g4d0DSxsw7OWBOTVWhwHr3hHHz35dXeZwxvoSWveZ7bkRvQLL4 au4SZ0MLch5ergwV2vN5gBWAFCLIbhsvXKg1j9K3pt+CoQv9dcAzCt2KJdHQWO/HZ4Qu /zEpZkKiLfuso73nRfjwdqjduc/hyXPL7aF8m0so7Q6kjFfMqLoY4btsBLOj7cD1sBCj 231xcCmoR5MSs35HjXmqst525hGXUWHW7pb8hr2pg519NK3Hv7u2vqrB/Ig3AXr5V2+Z DzFw== 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=KT4UMEDMfBliZq1v5DrhixRvNz8VWK1UssCvJfXzF+A=; b=uQi1Cax9Pg7Mi9RmxlOuI5GSeva5/WFd6pQVJsHDo+paR++V0i/bQxFjfreXj+KRzA a/PxieyC+KEKY3q14fZseF+IhUCiMEOjdxJsg8CfIrVgMimRDtAyl397I8DShk/6AACB obRS1LJXtjwjKW+1ETXGj3j6ztSoESg1BOG53NKeLRWTCkBQu1D2yh4KrdhaeuZPhWO+ LS8lfpESNmgg742tdMahjAYJBxN0Cg7Mf34EgYbH9dcEuYF5uZ64CHXx+iaEOuAZnUQL nnEM4skaa2U3M4ml+pky4xjf7IC50Cd6fLYQ903ZRqLx5JFc68YjIYhsnHQHcbENTIGK kujw== X-Gm-Message-State: APjAAAXP853d93TAaOSJeTPfWb5dR/GSjK1aiXqcqBPbZ+jS8yJ1c3tO k9Z+3VGSGfbiq/Lo8XpEShR9A9wyFy/imgjs95yoVQ== X-Google-Smtp-Source: APXvYqzbAMgEOdphgOUBwsn2wr1nNBKvv61PpLN8RqYqkcqXdVtaFoGwmF9EtbTYhsTyBSawiMI4wbRpNtUQ6lZTIlA= X-Received: by 2002:aa7:8518:: with SMTP id v24mr31090291pfn.219.1554742774344; Mon, 08 Apr 2019 09:59:34 -0700 (PDT) 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: Mon, 8 Apr 2019 19:59:23 +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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Message-ID: <20190408165923.hDpLKFiyFmd5Ev1ZpRZoXOP-0SzppOG3fGZhhhFwz40@z> Hi Ferruh, Can we proceed with this patch? Igor On Thu, Jan 24, 2019 at 9:05 PM Igor Ryzhov wrote: > 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 suppos= e > 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.pars= e >> 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#L2= 100 >> >> I saw your previous reply. That is why changed the question slightly, >> from '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 i= s >> a good >> reason to not do so. >> >> > That's why I think my patch should be accepted as is, and the problem = of >> > 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 enable= d >> 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 >> not 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 */ >> > > > >> > > >> > >> >>