DPDK patches and discussions
 help / color / mirror / Atom feed
From: Igor Ryzhov <iryzhov@nfware.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>
Cc: dev@dpdk.org, Stephen Hemminger <stephen@networkplumber.org>
Subject: Re: [dpdk-dev] [PATCH] kni: implement header_ops parse method
Date: Mon, 8 Apr 2019 19:59:23 +0300	[thread overview]
Message-ID: <CAF+s_FxxdV7HeGZN3wz6-KUvjrX_ymry9DYVxs5kVgRNrvJuqQ@mail.gmail.com> (raw)
In-Reply-To: <CAF+s_Fxz+WLSQ=hoq1c_4Y1JvpRfwJXvu1-C9psY=98PvnSVPQ@mail.gmail.com>

Hi Ferruh,

Can we proceed with this patch?

Igor

On Thu, Jan 24, 2019 at 9:05 PM Igor Ryzhov <iryzhov@nfware.com> 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 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 –
> 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 <ferruh.yigit@intel.com>
> 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 –
>> >
>> https://elixir.bootlin.com/linux/latest/source/net/packet/af_packet.c#L2100
>>
>> 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 is
>> 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 <ferruh.yigit@intel.com
>> > <mailto:ferruh.yigit@intel.com>> 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
>> 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 <iryzhov@nfware.com
>> >     <mailto:iryzhov@nfware.com>
>> >     > <mailto:iryzhov@nfware.com <mailto:iryzhov@nfware.com>>> 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 – 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
>> >     <mailto:ferruh.yigit@intel.com>
>> >     >     <mailto:ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>>>
>> wrote:
>> >     >
>> >     >         On 9/27/2018 1:02 AM, Igor Ryzhov wrote:
>> >     >         > Signed-off-by: Igor Ryzhov <iryzhov@nfware.com
>> >     <mailto:iryzhov@nfware.com>
>> >     >         <mailto:iryzhov@nfware.com <mailto:iryzhov@nfware.com>>>
>> >     >
>> >     >         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 = 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 = {
>> >     >         >       .create  = kni_net_header,
>> >     >         > +     .parse   = kni_net_header_parse,
>> >     >         >  #ifdef HAVE_REBUILD_HEADER
>> >     >         >       .rebuild = kni_net_rebuild_header,
>> >     >         >  #endif /* < 4.1.0  */
>> >     >         >
>> >     >
>> >
>>
>>

  reply	other threads:[~2019-04-08 16:59 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-27  0:02 Igor Ryzhov
2018-09-29  7:21 ` Stephen Hemminger
2018-09-29 19:19   ` Igor Ryzhov
2018-09-30  8:22     ` Igor Ryzhov
2018-10-02 16:58 ` Ferruh Yigit
2018-11-30 19:07   ` Igor Ryzhov
2019-01-24  9:18     ` Igor Ryzhov
2019-01-24 14:10       ` Ferruh Yigit
2019-01-24 16:35         ` Igor Ryzhov
2019-01-24 17:15           ` Ferruh Yigit
2019-01-24 18:05             ` Igor Ryzhov
2019-04-08 16:59               ` Igor Ryzhov [this message]
2019-04-08 16:59                 ` Igor Ryzhov
2019-04-09 20:30                 ` Ferruh Yigit
2019-04-09 20:30                   ` Ferruh Yigit
2019-04-10 10:30 ` [dpdk-dev] [PATCH v2] " Igor Ryzhov
2019-04-10 10:30   ` Igor Ryzhov
2019-04-12 14:52   ` Ferruh Yigit
2019-04-12 14:52     ` Ferruh Yigit
2019-04-12 14:53     ` Ferruh Yigit
2019-04-12 14:53       ` Ferruh Yigit
2019-04-12 17:12       ` Igor Ryzhov
2019-04-12 17:12         ` Igor Ryzhov
2019-04-12 17:15         ` Ferruh Yigit
2019-04-12 17:15           ` Ferruh Yigit
2019-04-15  8:37           ` Igor Ryzhov
2019-04-15  8:37             ` Igor Ryzhov
2019-04-15 15:49             ` Ferruh Yigit
2019-04-15 15:49               ` Ferruh Yigit
2019-04-19 10:36     ` Thomas Monjalon
2019-04-19 10:36       ` Thomas Monjalon

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=CAF+s_FxxdV7HeGZN3wz6-KUvjrX_ymry9DYVxs5kVgRNrvJuqQ@mail.gmail.com \
    --to=iryzhov@nfware.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=stephen@networkplumber.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).