On 5/28/20 1:01 AM, Andrii Nakryiko wrote: > > Please cc b...@vger.kernel.org in the future for patches related to BPF > in general.
added to my script > >> include/linux/bpf.h | 5 +++ >> include/uapi/linux/bpf.h | 5 +++ >> kernel/bpf/devmap.c | 79 +++++++++++++++++++++++++++++++++- >> net/core/dev.c | 18 ++++++++ >> tools/include/uapi/linux/bpf.h | 5 +++ >> 5 files changed, 110 insertions(+), 2 deletions(-) >> > > [...] > >> >> +static struct xdp_buff *dev_map_run_prog(struct net_device *dev, >> + struct xdp_buff *xdp, >> + struct bpf_prog *xdp_prog) >> +{ >> + u32 act; >> + >> + act = bpf_prog_run_xdp(xdp_prog, xdp); >> + switch (act) { >> + case XDP_DROP: >> + fallthrough; > > nit: I don't think fallthrough is necessary for cases like: > > case XDP_DROP: > case XDP_PASS: > /* do something */ > >> + case XDP_PASS: >> + break; >> + default: >> + bpf_warn_invalid_xdp_action(act); >> + fallthrough; >> + case XDP_ABORTED: >> + trace_xdp_exception(dev, xdp_prog, act); >> + act = XDP_DROP; >> + break; >> + } >> + >> + if (act == XDP_DROP) { >> + xdp_return_buff(xdp); >> + xdp = NULL; > > hm.. if you move XDP_DROP case to after XDP_ABORTED and do fallthrough > from XDP_ABORTED, you won't even need to override act and it will just > handle all the cases, no? > > switch (act) { > case XDP_PASS: > return xdp; > default: > bpf_warn_invalid_xdp_action(act); > fallthrough; > case XDP_ABORTED: > trace_xdp_exception(dev, xdp_prog, act); > fallthrough; > case XDP_DROP: > xdp_return_buff(xdp); > return NULL; > } > > Wouldn't this be simpler? > Switched it to this which captures your intent with a more traditional return location. act = bpf_prog_run_xdp(xdp_prog, xdp); switch (act) { case XDP_PASS: return xdp; case XDP_DROP: break; default: bpf_warn_invalid_xdp_action(act); fallthrough; case XDP_ABORTED: trace_xdp_exception(dev, xdp_prog, act); break; } xdp_return_buff(xdp); return NULL;