| Message ID | d8e0306b70b05bdd5e628459863d1027c8c26959.1503537400.git.daniel@iogearbox.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On 8/23/17 6:20 PM, Daniel Borkmann wrote: > No need to test for it in fast-path, every dev in bpf_dtab_netdev > is guaranteed to be non-NULL, otherwise dev_map_update_elem() will > fail in the first place. > > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> wow. interesting. I'm surprised you see a difference from such micro-optimization. Acked-by: Alexei Starovoitov <ast@kernel.org>
On 08/23/2017 06:25 PM, Alexei Starovoitov wrote: > On 8/23/17 6:20 PM, Daniel Borkmann wrote: >> No need to test for it in fast-path, every dev in bpf_dtab_netdev >> is guaranteed to be non-NULL, otherwise dev_map_update_elem() will >> fail in the first place. >> >> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> > > wow. interesting. I'm surprised you see a difference from > such micro-optimization. > Acked-by: Alexei Starovoitov <ast@kernel.org> > > Thanks for the clean up, I was a bit over paranoid here as well. Is this actually noticeable in pps benchmark or just making the code cleaner? Just curious. Acked-by: John Fastabend <john.fastabend@gmail.com>
From: Daniel Borkmann <daniel@iogearbox.net> Date: Thu, 24 Aug 2017 03:20:11 +0200 > No need to test for it in fast-path, every dev in bpf_dtab_netdev > is guaranteed to be non-NULL, otherwise dev_map_update_elem() will > fail in the first place. > > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Applied.
On 08/24/2017 05:10 AM, John Fastabend wrote: > On 08/23/2017 06:25 PM, Alexei Starovoitov wrote: >> On 8/23/17 6:20 PM, Daniel Borkmann wrote: >>> No need to test for it in fast-path, every dev in bpf_dtab_netdev >>> is guaranteed to be non-NULL, otherwise dev_map_update_elem() will >>> fail in the first place. >>> >>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> >> >> wow. interesting. I'm surprised you see a difference from >> such micro-optimization. >> Acked-by: Alexei Starovoitov <ast@kernel.org> > > Thanks for the clean up, I was a bit over paranoid here as well. Is > this actually noticeable in pps benchmark or just making the code > cleaner? Just curious. No, just from going over the code wondering why it could be null here but not elsewhere.
diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c index bfecabf..ecf9f99 100644 --- a/kernel/bpf/devmap.c +++ b/kernel/bpf/devmap.c @@ -226,12 +226,10 @@ void __dev_map_flush(struct bpf_map *map) if (unlikely(!dev)) continue; - netdev = dev->dev; __clear_bit(bit, bitmap); - if (unlikely(!netdev || !netdev->netdev_ops->ndo_xdp_flush)) - continue; - - netdev->netdev_ops->ndo_xdp_flush(netdev); + netdev = dev->dev; + if (likely(netdev->netdev_ops->ndo_xdp_flush)) + netdev->netdev_ops->ndo_xdp_flush(netdev); } }
No need to test for it in fast-path, every dev in bpf_dtab_netdev is guaranteed to be non-NULL, otherwise dev_map_update_elem() will fail in the first place. Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> --- kernel/bpf/devmap.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)