[dpdk-dev] [PATCH] kni: implement header_ops parse method
Igor Ryzhov
iryzhov at nfware.com
Thu Jan 24 19:05:31 CET 2019
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 at 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 at intel.com
> > <mailto:ferruh.yigit at 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 at nfware.com
> > <mailto:iryzhov at nfware.com>
> > > <mailto:iryzhov at nfware.com <mailto:iryzhov at 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 at intel.com
> > <mailto:ferruh.yigit at intel.com>
> > > <mailto:ferruh.yigit at intel.com <mailto:ferruh.yigit at intel.com>>>
> wrote:
> > >
> > > On 9/27/2018 1:02 AM, Igor Ryzhov wrote:
> > > > Signed-off-by: Igor Ryzhov <iryzhov at nfware.com
> > <mailto:iryzhov at nfware.com>
> > > <mailto:iryzhov at nfware.com <mailto:iryzhov at 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 */
> > > >
> > >
> >
>
>
More information about the dev
mailing list