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

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 4/8/2019 5:59 PM, Igor Ryzhov wrote:
> Hi Ferruh,
> 
> Can we proceed with this patch?
> 
> Igor
> 
> On Thu, Jan 24, 2019 at 9:05 PM Igor Ryzhov <iryzhov@nfware.com
> <mailto: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.

Hi Igor,

Sorry for the delay, thanks for the clarification on patch.
Can you please send v2 with 'eth_header_parse'? And please include above
information in commit log.

Thanks,
ferruh

> 
>     Best regards,
>     Igor
> 
>     On Thu, Jan 24, 2019 at 8:15 PM Ferruh Yigit <ferruh.yigit@intel.com
>     <mailto: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>
>         > <mailto: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>>
>         >     > <mailto: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>>
>         >     >     <mailto: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>>
>         >     >         <mailto: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  */
>         >     >         >
>         >     >
>         >
>