| Message ID | 1606143915-25335-1-git-send-email-yanjunz@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/1] xdp: remove the function xsk_map_inc | expand |
On Mon, Nov 23, 2020 at 4:07 PM Zhu Yanjun <yanjunz@nvidia.com> wrote: > > From: Zhu Yanjun <zyjzyj2000@gmail.com> > > The function xsk_map_inc is a simple wrapper of bpf_map_inc and > always returns zero. As such, replacing this function with bpf_map_inc > and removing the test code. Applied it to bpf-next, compiled it and tested it and it passed. Thank you Yanjun! Acked-by: Magnus Karlsson <magnus.karlsson@intel.com> > Signed-off-by: Zhu Yanjun <zyjzyj2000@gmail.com> > --- > net/xdp/xsk.c | 2 +- > net/xdp/xsk.h | 1 - > net/xdp/xskmap.c | 13 +------------ > 3 files changed, 2 insertions(+), 14 deletions(-) > > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c > index cfbec3989a76..a3c1f07d77d8 100644 > --- a/net/xdp/xsk.c > +++ b/net/xdp/xsk.c > @@ -548,7 +548,7 @@ static struct xsk_map *xsk_get_map_list_entry(struct xdp_sock *xs, > node = list_first_entry_or_null(&xs->map_list, struct xsk_map_node, > node); > if (node) { > - WARN_ON(xsk_map_inc(node->map)); > + bpf_map_inc(&node->map->map); > map = node->map; > *map_entry = node->map_entry; > } > diff --git a/net/xdp/xsk.h b/net/xdp/xsk.h > index b9e896cee5bb..0aad25c0e223 100644 > --- a/net/xdp/xsk.h > +++ b/net/xdp/xsk.h > @@ -41,7 +41,6 @@ static inline struct xdp_sock *xdp_sk(struct sock *sk) > > void xsk_map_try_sock_delete(struct xsk_map *map, struct xdp_sock *xs, > struct xdp_sock **map_entry); > -int xsk_map_inc(struct xsk_map *map); > void xsk_map_put(struct xsk_map *map); > void xsk_clear_pool_at_qid(struct net_device *dev, u16 queue_id); > int xsk_reg_pool_at_qid(struct net_device *dev, struct xsk_buff_pool *pool, > diff --git a/net/xdp/xskmap.c b/net/xdp/xskmap.c > index 49da2b8ace8b..6b7e9a72b101 100644 > --- a/net/xdp/xskmap.c > +++ b/net/xdp/xskmap.c > @@ -11,12 +11,6 @@ > > #include "xsk.h" > > -int xsk_map_inc(struct xsk_map *map) > -{ > - bpf_map_inc(&map->map); > - return 0; > -} > - > void xsk_map_put(struct xsk_map *map) > { > bpf_map_put(&map->map); > @@ -26,17 +20,12 @@ static struct xsk_map_node *xsk_map_node_alloc(struct xsk_map *map, > struct xdp_sock **map_entry) > { > struct xsk_map_node *node; > - int err; > > node = kzalloc(sizeof(*node), GFP_ATOMIC | __GFP_NOWARN); > if (!node) > return ERR_PTR(-ENOMEM); > > - err = xsk_map_inc(map); > - if (err) { > - kfree(node); > - return ERR_PTR(err); > - } > + bpf_map_inc(&map->map); > > node->map = map; > node->map_entry = map_entry; > -- > 2.25.1 >
On 11/23/20 4:05 PM, Zhu Yanjun wrote: > From: Zhu Yanjun <zyjzyj2000@gmail.com> > > The function xsk_map_inc is a simple wrapper of bpf_map_inc and > always returns zero. As such, replacing this function with bpf_map_inc > and removing the test code. > > Signed-off-by: Zhu Yanjun <zyjzyj2000@gmail.com> > --- > net/xdp/xsk.c | 2 +- > net/xdp/xsk.h | 1 - > net/xdp/xskmap.c | 13 +------------ > 3 files changed, 2 insertions(+), 14 deletions(-) > > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c > index cfbec3989a76..a3c1f07d77d8 100644 > --- a/net/xdp/xsk.c > +++ b/net/xdp/xsk.c > @@ -548,7 +548,7 @@ static struct xsk_map *xsk_get_map_list_entry(struct xdp_sock *xs, > node = list_first_entry_or_null(&xs->map_list, struct xsk_map_node, > node); > if (node) { > - WARN_ON(xsk_map_inc(node->map)); > + bpf_map_inc(&node->map->map); > map = node->map; > *map_entry = node->map_entry; > } > diff --git a/net/xdp/xsk.h b/net/xdp/xsk.h > index b9e896cee5bb..0aad25c0e223 100644 > --- a/net/xdp/xsk.h > +++ b/net/xdp/xsk.h > @@ -41,7 +41,6 @@ static inline struct xdp_sock *xdp_sk(struct sock *sk) > > void xsk_map_try_sock_delete(struct xsk_map *map, struct xdp_sock *xs, > struct xdp_sock **map_entry); > -int xsk_map_inc(struct xsk_map *map); > void xsk_map_put(struct xsk_map *map); > void xsk_clear_pool_at_qid(struct net_device *dev, u16 queue_id); > int xsk_reg_pool_at_qid(struct net_device *dev, struct xsk_buff_pool *pool, > diff --git a/net/xdp/xskmap.c b/net/xdp/xskmap.c > index 49da2b8ace8b..6b7e9a72b101 100644 > --- a/net/xdp/xskmap.c > +++ b/net/xdp/xskmap.c > @@ -11,12 +11,6 @@ > > #include "xsk.h" > > -int xsk_map_inc(struct xsk_map *map) > -{ > - bpf_map_inc(&map->map); > - return 0; > -} > - > void xsk_map_put(struct xsk_map *map) > { So, the xsk_map_put() is defined as: void xsk_map_put(struct xsk_map *map) { bpf_map_put(&map->map); } What is the reason to get rid of xsk_map_inc() but not xsk_map_put() wrapper? Can't we just remove both while we're at it? Thanks, Daniel
On Wed, Nov 25, 2020 at 1:02 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 11/23/20 4:05 PM, Zhu Yanjun wrote: > > From: Zhu Yanjun <zyjzyj2000@gmail.com> > > > > The function xsk_map_inc is a simple wrapper of bpf_map_inc and > > always returns zero. As such, replacing this function with bpf_map_inc > > and removing the test code. > > > > Signed-off-by: Zhu Yanjun <zyjzyj2000@gmail.com> > > --- > > net/xdp/xsk.c | 2 +- > > net/xdp/xsk.h | 1 - > > net/xdp/xskmap.c | 13 +------------ > > 3 files changed, 2 insertions(+), 14 deletions(-) > > > > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c > > index cfbec3989a76..a3c1f07d77d8 100644 > > --- a/net/xdp/xsk.c > > +++ b/net/xdp/xsk.c > > @@ -548,7 +548,7 @@ static struct xsk_map *xsk_get_map_list_entry(struct xdp_sock *xs, > > node = list_first_entry_or_null(&xs->map_list, struct xsk_map_node, > > node); > > if (node) { > > - WARN_ON(xsk_map_inc(node->map)); > > + bpf_map_inc(&node->map->map); > > map = node->map; > > *map_entry = node->map_entry; > > } > > diff --git a/net/xdp/xsk.h b/net/xdp/xsk.h > > index b9e896cee5bb..0aad25c0e223 100644 > > --- a/net/xdp/xsk.h > > +++ b/net/xdp/xsk.h > > @@ -41,7 +41,6 @@ static inline struct xdp_sock *xdp_sk(struct sock *sk) > > > > void xsk_map_try_sock_delete(struct xsk_map *map, struct xdp_sock *xs, > > struct xdp_sock **map_entry); > > -int xsk_map_inc(struct xsk_map *map); > > void xsk_map_put(struct xsk_map *map); > > void xsk_clear_pool_at_qid(struct net_device *dev, u16 queue_id); > > int xsk_reg_pool_at_qid(struct net_device *dev, struct xsk_buff_pool *pool, > > diff --git a/net/xdp/xskmap.c b/net/xdp/xskmap.c > > index 49da2b8ace8b..6b7e9a72b101 100644 > > --- a/net/xdp/xskmap.c > > +++ b/net/xdp/xskmap.c > > @@ -11,12 +11,6 @@ > > > > #include "xsk.h" > > > > -int xsk_map_inc(struct xsk_map *map) > > -{ > > - bpf_map_inc(&map->map); > > - return 0; > > -} > > - > > void xsk_map_put(struct xsk_map *map) > > { > > So, the xsk_map_put() is defined as: > > void xsk_map_put(struct xsk_map *map) > { > bpf_map_put(&map->map); > } > > What is the reason to get rid of xsk_map_inc() but not xsk_map_put() wrapper? > Can't we just remove both while we're at it? Yes, why not. Makes sense. Yanjun, could you please send a new version that removes this too? Thank you both! > Thanks, > Daniel
On Wed, Nov 25, 2020 at 4:33 PM Magnus Karlsson <magnus.karlsson@gmail.com> wrote: > > On Wed, Nov 25, 2020 at 1:02 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > > > > On 11/23/20 4:05 PM, Zhu Yanjun wrote: > > > From: Zhu Yanjun <zyjzyj2000@gmail.com> > > > > > > The function xsk_map_inc is a simple wrapper of bpf_map_inc and > > > always returns zero. As such, replacing this function with bpf_map_inc > > > and removing the test code. > > > > > > Signed-off-by: Zhu Yanjun <zyjzyj2000@gmail.com> > > > --- > > > net/xdp/xsk.c | 2 +- > > > net/xdp/xsk.h | 1 - > > > net/xdp/xskmap.c | 13 +------------ > > > 3 files changed, 2 insertions(+), 14 deletions(-) > > > > > > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c > > > index cfbec3989a76..a3c1f07d77d8 100644 > > > --- a/net/xdp/xsk.c > > > +++ b/net/xdp/xsk.c > > > @@ -548,7 +548,7 @@ static struct xsk_map *xsk_get_map_list_entry(struct xdp_sock *xs, > > > node = list_first_entry_or_null(&xs->map_list, struct xsk_map_node, > > > node); > > > if (node) { > > > - WARN_ON(xsk_map_inc(node->map)); > > > + bpf_map_inc(&node->map->map); > > > map = node->map; > > > *map_entry = node->map_entry; > > > } > > > diff --git a/net/xdp/xsk.h b/net/xdp/xsk.h > > > index b9e896cee5bb..0aad25c0e223 100644 > > > --- a/net/xdp/xsk.h > > > +++ b/net/xdp/xsk.h > > > @@ -41,7 +41,6 @@ static inline struct xdp_sock *xdp_sk(struct sock *sk) > > > > > > void xsk_map_try_sock_delete(struct xsk_map *map, struct xdp_sock *xs, > > > struct xdp_sock **map_entry); > > > -int xsk_map_inc(struct xsk_map *map); > > > void xsk_map_put(struct xsk_map *map); > > > void xsk_clear_pool_at_qid(struct net_device *dev, u16 queue_id); > > > int xsk_reg_pool_at_qid(struct net_device *dev, struct xsk_buff_pool *pool, > > > diff --git a/net/xdp/xskmap.c b/net/xdp/xskmap.c > > > index 49da2b8ace8b..6b7e9a72b101 100644 > > > --- a/net/xdp/xskmap.c > > > +++ b/net/xdp/xskmap.c > > > @@ -11,12 +11,6 @@ > > > > > > #include "xsk.h" > > > > > > -int xsk_map_inc(struct xsk_map *map) > > > -{ > > > - bpf_map_inc(&map->map); > > > - return 0; > > > -} > > > - > > > void xsk_map_put(struct xsk_map *map) > > > { > > > > So, the xsk_map_put() is defined as: > > > > void xsk_map_put(struct xsk_map *map) > > { > > bpf_map_put(&map->map); > > } > > > > What is the reason to get rid of xsk_map_inc() but not xsk_map_put() wrapper? > > Can't we just remove both while we're at it? > > Yes, why not. Makes sense. > > Yanjun, could you please send a new version that removes this too? OK. I will. Zhu Yanjun > > Thank you both! > > > Thanks, > > Daniel
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c index cfbec3989a76..a3c1f07d77d8 100644 --- a/net/xdp/xsk.c +++ b/net/xdp/xsk.c @@ -548,7 +548,7 @@ static struct xsk_map *xsk_get_map_list_entry(struct xdp_sock *xs, node = list_first_entry_or_null(&xs->map_list, struct xsk_map_node, node); if (node) { - WARN_ON(xsk_map_inc(node->map)); + bpf_map_inc(&node->map->map); map = node->map; *map_entry = node->map_entry; } diff --git a/net/xdp/xsk.h b/net/xdp/xsk.h index b9e896cee5bb..0aad25c0e223 100644 --- a/net/xdp/xsk.h +++ b/net/xdp/xsk.h @@ -41,7 +41,6 @@ static inline struct xdp_sock *xdp_sk(struct sock *sk) void xsk_map_try_sock_delete(struct xsk_map *map, struct xdp_sock *xs, struct xdp_sock **map_entry); -int xsk_map_inc(struct xsk_map *map); void xsk_map_put(struct xsk_map *map); void xsk_clear_pool_at_qid(struct net_device *dev, u16 queue_id); int xsk_reg_pool_at_qid(struct net_device *dev, struct xsk_buff_pool *pool, diff --git a/net/xdp/xskmap.c b/net/xdp/xskmap.c index 49da2b8ace8b..6b7e9a72b101 100644 --- a/net/xdp/xskmap.c +++ b/net/xdp/xskmap.c @@ -11,12 +11,6 @@ #include "xsk.h" -int xsk_map_inc(struct xsk_map *map) -{ - bpf_map_inc(&map->map); - return 0; -} - void xsk_map_put(struct xsk_map *map) { bpf_map_put(&map->map); @@ -26,17 +20,12 @@ static struct xsk_map_node *xsk_map_node_alloc(struct xsk_map *map, struct xdp_sock **map_entry) { struct xsk_map_node *node; - int err; node = kzalloc(sizeof(*node), GFP_ATOMIC | __GFP_NOWARN); if (!node) return ERR_PTR(-ENOMEM); - err = xsk_map_inc(map); - if (err) { - kfree(node); - return ERR_PTR(err); - } + bpf_map_inc(&map->map); node->map = map; node->map_entry = map_entry;