Re: [dpdk-dev] [PATCH] kni: implement header_ops parse method

[Igor Ryzhov <iryzhov@nfware.com>]

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

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.
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.

Best regards,
Igor

On Thu, Jan 24, 2019 at 5:10 PM Ferruh Yigit <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>> 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>> wrote:
> >
> >         On 9/27/2018 1:02 AM, Igor Ryzhov wrote:
> >         > Signed-off-by: Igor Ryzhov <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  */
> >         >
> >
>
>