Re: [PATCH v2 net-next 8/9] bpf: nfp: Report bpf_prog ID during XDP_QUERY_PROG

[Jakub Kicinski]

On Tue, 13 Jun 2017 20:19:53 -0700, Martin KaFai Lau wrote:
> On Tue, Jun 13, 2017 at 07:19:50PM -0700, Jakub Kicinski wrote:
> > On Tue, 13 Jun 2017 17:37:50 -0700, Martin KaFai Lau wrote:  
> > > On Tue, Jun 13, 2017 at 04:52:32PM -0700, Jakub Kicinski wrote:  
> > > > On Tue, 13 Jun 2017 14:08:40 -0700, Martin KaFai Lau wrote:  
> > > > > - case XDP_QUERY_PROG:
> > > > > - xdp->prog_attached = !!nn->dp.xdp_prog;
> > > > > + case XDP_QUERY_PROG: {
> > > > > + const struct bpf_prog *xdp_prog;
> > > > > +
> > > > > + xdp_prog = nn->dp.xdp_prog;
> > > > > + if (xdp_prog) {
> > > > > + xdp->prog_id = xdp_prog->aux->id;
> > > > > + xdp->prog_attached = true;
> > > > > + } else {
> > > > > + xdp->prog_id = 0;
> > > > > + xdp->prog_attached = false;
> > > > > + }
> > > > >   return 0;
> > > > > + }  
> > > >
> > > > I'm sorry to nit pick but it could be done on a single line:
> > > >
> > > > case XDP_QUERY_PROG:
> > > > xdp->prog_attached = !!nn->dp.xdp_prog;
> > > > +   xdp->prog_id = nn->dp.xdp_prog ? 
> > > > nn->dp.xdp_prog->aux->id : 0;
> > > > return 0;
> > > > default:
> > > > return -EINVAL;  
> > > OK...
> > >  
> > > >
> > > >
> > > > What would be even cooler is a helper like this:
> > > >
> > > > static inline u32 bpf_prog_id(struct bpf_prog *prog)
> > > > {
> > > > if (!prog)
> > > > return 0;
> > > > return prog->aux->id;
> > > > }
> > > >
> > > > in linux/bpf.h.  
> > > Good idea.  
> >
> > You may actually have to add that into a source file, because bpf.h
> > does not know the definition of struct bpf_prog :(  
> Yeah. filter.h seems not working well either.
> It looks good to me at the first thought.  After a second thought,
> in the future, I am not so sure having a getter for every fields
> in bpf_prog.

Yes, if it's not a static inline it's far less appealing...
 
> I can put bpf_prog_id() in nfp_net_common.c only.
> or do 'xdp->prog_id = nn->dp.xdp_prog ? nn->dp.xdp_prog->aux->id : 0;'
> as you suggested earlier also.
> I am fine either way.  Your call ;)

OK, let's do the ternary operator version then :)  Sorry for leading you
into this dead end.

Re: [PATCH v2 net-next 8/9] bpf: nfp: Report bpf_prog ID during XDP_QUERY_PROG

[Martin KaFai Lau]

On Tue, Jun 13, 2017 at 07:19:50PM -0700, Jakub Kicinski wrote:
> On Tue, 13 Jun 2017 17:37:50 -0700, Martin KaFai Lau wrote:
> > On Tue, Jun 13, 2017 at 04:52:32PM -0700, Jakub Kicinski wrote:
> > > On Tue, 13 Jun 2017 14:08:40 -0700, Martin KaFai Lau wrote:
> > > > -   case XDP_QUERY_PROG:
> > > > -   xdp->prog_attached = !!nn->dp.xdp_prog;
> > > > +   case XDP_QUERY_PROG: {
> > > > +   const struct bpf_prog *xdp_prog;
> > > > +
> > > > +   xdp_prog = nn->dp.xdp_prog;
> > > > +   if (xdp_prog) {
> > > > +   xdp->prog_id = xdp_prog->aux->id;
> > > > +   xdp->prog_attached = true;
> > > > +   } else {
> > > > +   xdp->prog_id = 0;
> > > > +   xdp->prog_attached = false;
> > > > +   }
> > > > return 0;
> > > > +   }
> > >
> > > I'm sorry to nit pick but it could be done on a single line:
> > >
> > >   case XDP_QUERY_PROG:
> > >   xdp->prog_attached = !!nn->dp.xdp_prog;
> > > + xdp->prog_id = nn->dp.xdp_prog ? nn->dp.xdp_prog->aux->id : 0;
> > >   return 0;
> > >   default:
> > >   return -EINVAL;
> > OK...
> >
> > >
> > >
> > > What would be even cooler is a helper like this:
> > >
> > > static inline u32 bpf_prog_id(struct bpf_prog *prog)
> > > {
> > >   if (!prog)
> > >   return 0;
> > >   return prog->aux->id;
> > > }
> > >
> > > in linux/bpf.h.
> > Good idea.
>
> You may actually have to add that into a source file, because bpf.h
> does not know the definition of struct bpf_prog :(
Yeah. filter.h seems not working well either.
It looks good to me at the first thought.  After a second thought,
in the future, I am not so sure having a getter for every fields
in bpf_prog.

I can put bpf_prog_id() in nfp_net_common.c only.
or do 'xdp->prog_id = nn->dp.xdp_prog ? nn->dp.xdp_prog->aux->id : 0;'
as you suggested earlier also.
I am fine either way.  Your call ;)

>
> > I had been thinking I may not need to change all the
> > drivers now.  I did that in v1 because I wanted to remove
> > prog_attached which is redundant.  With prog_attached reserved,
> > prog_id is optional.
> >
> > Considering I don't have all the hardwares to test it,  I think
> > it may make more sense for me to only change the HW that I have?
>
> Coccinelle to the rescue?
>
> @@
> expression ex;
> @@
>   xdp->prog_attached = !!(ex);
> + xdp->prog_id = bpf_prog_id(ex);
Good to know Coccinelle.  First hit in my browser ;)

Changing it is fine.  I meant I cannot test it without the HW but
they are at least compiler tested now.

Regardless, I think I will give it one more try in v3.

Re: [PATCH v2 net-next 8/9] bpf: nfp: Report bpf_prog ID during XDP_QUERY_PROG

[Jakub Kicinski]

On Tue, 13 Jun 2017 17:37:50 -0700, Martin KaFai Lau wrote:
> On Tue, Jun 13, 2017 at 04:52:32PM -0700, Jakub Kicinski wrote:
> > On Tue, 13 Jun 2017 14:08:40 -0700, Martin KaFai Lau wrote:  
> > > - case XDP_QUERY_PROG:
> > > - xdp->prog_attached = !!nn->dp.xdp_prog;
> > > + case XDP_QUERY_PROG: {
> > > + const struct bpf_prog *xdp_prog;
> > > +
> > > + xdp_prog = nn->dp.xdp_prog;
> > > + if (xdp_prog) {
> > > + xdp->prog_id = xdp_prog->aux->id;
> > > + xdp->prog_attached = true;
> > > + } else {
> > > + xdp->prog_id = 0;
> > > + xdp->prog_attached = false;
> > > + }
> > >   return 0;
> > > + }  
> >
> > I'm sorry to nit pick but it could be done on a single line:
> >
> > case XDP_QUERY_PROG:
> > xdp->prog_attached = !!nn->dp.xdp_prog;
> > +   xdp->prog_id = nn->dp.xdp_prog ? nn->dp.xdp_prog->aux->id : 0;
> > return 0;
> > default:
> > return -EINVAL;  
> OK...
> 
> >
> >
> > What would be even cooler is a helper like this:
> >
> > static inline u32 bpf_prog_id(struct bpf_prog *prog)
> > {
> > if (!prog)
> > return 0;
> > return prog->aux->id;
> > }
> >
> > in linux/bpf.h.
> Good idea.

You may actually have to add that into a source file, because bpf.h
does not know the definition of struct bpf_prog :(

> I had been thinking I may not need to change all the
> drivers now.  I did that in v1 because I wanted to remove
> prog_attached which is redundant.  With prog_attached reserved,
> prog_id is optional.
> 
> Considering I don't have all the hardwares to test it,  I think
> it may make more sense for me to only change the HW that I have?

Coccinelle to the rescue?

@@
expression ex;
@@
xdp->prog_attached = !!(ex);
+   xdp->prog_id = bpf_prog_id(ex);

> > In patch 1 I would be tempted to add a new command for getting the prog
> > id, instead of muxing through query to avoid the output parameter?  But
> > I'm OK with the code as is, its just a preference rather than an objection 
> > :)  
> Have one command to query a new field?  I think it is overkilled.

Perhaps.  I was just trying to come up with a way of avoiding the
output parameter.  It seems hard to do unless we stop using
__dev_xdp_attached() for filling in the netlink attributes.  I'm OK
with leaving the code as is..

Re: [PATCH v2 net-next 8/9] bpf: nfp: Report bpf_prog ID during XDP_QUERY_PROG

[Martin KaFai Lau]

On Tue, Jun 13, 2017 at 04:52:32PM -0700, Jakub Kicinski wrote:
> On Tue, 13 Jun 2017 14:08:40 -0700, Martin KaFai Lau wrote:
> > -   case XDP_QUERY_PROG:
> > -   xdp->prog_attached = !!nn->dp.xdp_prog;
> > +   case XDP_QUERY_PROG: {
> > +   const struct bpf_prog *xdp_prog;
> > +
> > +   xdp_prog = nn->dp.xdp_prog;
> > +   if (xdp_prog) {
> > +   xdp->prog_id = xdp_prog->aux->id;
> > +   xdp->prog_attached = true;
> > +   } else {
> > +   xdp->prog_id = 0;
> > +   xdp->prog_attached = false;
> > +   }
> > return 0;
> > +   }
>
> I'm sorry to nit pick but it could be done on a single line:
>
>   case XDP_QUERY_PROG:
>   xdp->prog_attached = !!nn->dp.xdp_prog;
> + xdp->prog_id = nn->dp.xdp_prog ? nn->dp.xdp_prog->aux->id : 0;
>   return 0;
>   default:
>   return -EINVAL;
OK...

>
>
> What would be even cooler is a helper like this:
>
> static inline u32 bpf_prog_id(struct bpf_prog *prog)
> {
>   if (!prog)
>   return 0;
>   return prog->aux->id;
> }
>
> in linux/bpf.h.
Good idea.



I had been thinking I may not need to change all the
drivers now.  I did that in v1 because I wanted to remove
prog_attached which is redundant.  With prog_attached reserved,
prog_id is optional.

Considering I don't have all the hardwares to test it,  I think
it may make more sense for me to only change the HW that I have?

>
> In patch 1 I would be tempted to add a new command for getting the prog
> id, instead of muxing through query to avoid the output parameter?  But
> I'm OK with the code as is, its just a preference rather than an objection :)
Have one command to query a new field?  I think it is overkilled.

Re: [PATCH v2 net-next 8/9] bpf: nfp: Report bpf_prog ID during XDP_QUERY_PROG

[Jakub Kicinski]

On Tue, 13 Jun 2017 14:08:40 -0700, Martin KaFai Lau wrote:
> - case XDP_QUERY_PROG:
> - xdp->prog_attached = !!nn->dp.xdp_prog;
> + case XDP_QUERY_PROG: {
> + const struct bpf_prog *xdp_prog;
> +
> + xdp_prog = nn->dp.xdp_prog;
> + if (xdp_prog) {
> + xdp->prog_id = xdp_prog->aux->id;
> + xdp->prog_attached = true;
> + } else {
> + xdp->prog_id = 0;
> + xdp->prog_attached = false;
> + }
>   return 0;
> + }

I'm sorry to nit pick but it could be done on a single line:

case XDP_QUERY_PROG:
xdp->prog_attached = !!nn->dp.xdp_prog;
+   xdp->prog_id = nn->dp.xdp_prog ? nn->dp.xdp_prog->aux->id : 0;
return 0;
default:
return -EINVAL;


What would be even cooler is a helper like this:

static inline u32 bpf_prog_id(struct bpf_prog *prog)
{
if (!prog)
return 0;
return prog->aux->id;
}

in linux/bpf.h.  

In patch 1 I would be tempted to add a new command for getting the prog
id, instead of muxing through query to avoid the output parameter?  But
I'm OK with the code as is, its just a preference rather than an objection :)

[PATCH v2 net-next 8/9] bpf: nfp: Report bpf_prog ID during XDP_QUERY_PROG

[Martin KaFai Lau]

Add support to nfp to report bpf_prog ID during XDP_QUERY_PROG.

Signed-off-by: Martin KaFai Lau 
Cc: Jakub Kicinski 
Acked-by: Alexei Starovoitov 
Acked-by: Daniel Borkmann 
---
 drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c 
b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
index 49d1756d6a8e..272354fb0f13 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
@@ -3254,9 +3254,19 @@ static int nfp_net_xdp(struct net_device *netdev, struct 
netdev_xdp *xdp)
switch (xdp->command) {
case XDP_SETUP_PROG:
return nfp_net_xdp_setup(nn, xdp);
-   case XDP_QUERY_PROG:
-   xdp->prog_attached = !!nn->dp.xdp_prog;
+   case XDP_QUERY_PROG: {
+   const struct bpf_prog *xdp_prog;
+
+   xdp_prog = nn->dp.xdp_prog;
+   if (xdp_prog) {
+   xdp->prog_id = xdp_prog->aux->id;
+   xdp->prog_attached = true;
+   } else {
+   xdp->prog_id = 0;
+   xdp->prog_attached = false;
+   }
return 0;
+   }
default:
return -EINVAL;
}
-- 
2.9.3