| Message ID | 20170613210841.509578-9-kafai@fb.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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 :)
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.
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..
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.
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.
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; }