Re: [RFC 5/7] net: Add allocation flag to rtnl_unicast()

[Eric Dumazet]

On Fri, 2016-07-08 at 12:15 +0900, Masashi Honma wrote:
=
> Thanks for comment.
> 
> I have selected GFP flags based on existing code.
> 
> I have selected GFP_ATOMIC in inet6_netconf_get_devconf() because
> skb was allocated with GFP_ATOMIC.

Point is : we should remove GFP_ATOMIC uses as much as we can.

Everytime we see one of them, we should think why it was added
and if this is really needed.

inet6_netconf_get_devconf() is a perfect example of one careless
GFP_ATOMIC usage

https://patchwork.ozlabs.org/patch/646291/

Re: [RFC 5/7] net: Add allocation flag to rtnl_unicast()

[Masashi Honma]

On 2016年07月08日 11:56, Eric Dumazet wrote:


Managing to mix GFP_ATOMIC and GFP_KERNEL almost randomly as you did in
this patch is definitely not good.

Further more, RTNL is a mutex, held in control path, designed to allow
schedules and waiting for memory under pressure.

We do not want to encourage GFP_ATOMIC usage in control path.

Your patch series gives the wrong signal to developers.





Thanks for comment.

I have selected GFP flags based on existing code.

I have selected GFP_ATOMIC in inet6_netconf_get_devconf() because
skb was allocated with GFP_ATOMIC.

I have used GFP_KERNEL in inet6_rtm_getaddr() by same reason.

> I will send a patch against net/ipv4/devinet.c so that we remove
> GFP_ATOMIC usage there.

Thanks. I will modify my patch based on your new code.

Re: [RFC 5/7] net: Add allocation flag to rtnl_unicast()

[Eric Dumazet]

On Wed, 2016-07-06 at 09:28 +0900, Masashi Honma wrote:
> Signed-off-by: Masashi Honma 
> ---


> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index a1f6b7b..2b0b994 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -628,7 +628,7 @@ static int inet6_netconf_get_devconf(struct sk_buff 
> *in_skb,
>   kfree_skb(skb);
>   goto errout;
>   }
> - err = rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid);
> + err = rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid, GFP_ATOMIC);
>  errout:
>   return err;
>  }
> @@ -4824,7 +4824,7 @@ static int inet6_rtm_getaddr(struct sk_buff *in_skb, 
> struct nlmsghdr *nlh)
>   kfree_skb(skb);
>   goto errout_ifa;
>   }
> - err = rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid);
> + err = rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid, GFP_KERNEL);
>  errout_ifa:
>   in6_ifa_put(ifa);
>  errout:


Managing to mix GFP_ATOMIC and GFP_KERNEL almost randomly as you did in
this patch is definitely not good.

Further more, RTNL is a mutex, held in control path, designed to allow
schedules and waiting for memory under pressure.

We do not want to encourage GFP_ATOMIC usage in control path.

Your patch series gives the wrong signal to developers.

I will send a patch against net/ipv4/devinet.c so that we remove
GFP_ATOMIC usage there.

[RFC 5/7] net: Add allocation flag to rtnl_unicast()

[Masashi Honma]

Signed-off-by: Masashi Honma 
---
 include/linux/rtnetlink.h |  3 ++-
 net/core/net_namespace.c  |  2 +-
 net/core/rtnetlink.c  | 10 ++
 net/dcb/dcbnl.c   |  2 +-
 net/decnet/dn_route.c |  3 ++-
 net/ipv4/devinet.c|  2 +-
 net/ipv4/ipmr.c   |  6 --
 net/ipv4/route.c  |  2 +-
 net/ipv6/addrconf.c   |  4 ++--
 net/ipv6/addrlabel.c  |  2 +-
 net/ipv6/ip6mr.c  |  6 --
 net/ipv6/route.c  |  2 +-
 net/sched/act_api.c   |  2 +-
 13 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
index 2daece8..132730f 100644
--- a/include/linux/rtnetlink.h
+++ b/include/linux/rtnetlink.h
@@ -8,7 +8,8 @@
 #include 
 
 extern int rtnetlink_send(struct sk_buff *skb, struct net *net, u32 pid, u32 
group, int echo);
-extern int rtnl_unicast(struct sk_buff *skb, struct net *net, u32 pid);
+extern int rtnl_unicast(struct sk_buff *skb, struct net *net, u32 pid,
+   gfp_t flags);
 extern void rtnl_notify(struct sk_buff *skb, struct net *net, u32 pid,
u32 group, struct nlmsghdr *nlh, gfp_t flags);
 extern void rtnl_set_sk_err(struct net *net, u32 group, int error);
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index 2c2eb1b..28eed58 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -646,7 +646,7 @@ static int rtnl_net_getid(struct sk_buff *skb, struct 
nlmsghdr *nlh)
if (err < 0)
goto err_out;
 
-   err = rtnl_unicast(msg, net, NETLINK_CB(skb).portid);
+   err = rtnl_unicast(msg, net, NETLINK_CB(skb).portid, GFP_KERNEL);
goto out;
 
 err_out:
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 7f7927f..89fd826 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -653,11 +653,11 @@ int rtnetlink_send(struct sk_buff *skb, struct net *net, 
u32 pid, unsigned int g
return err;
 }
 
-int rtnl_unicast(struct sk_buff *skb, struct net *net, u32 pid)
+int rtnl_unicast(struct sk_buff *skb, struct net *net, u32 pid, gfp_t flags)
 {
struct sock *rtnl = net->rtnl;
 
-   return nlmsg_unicast(rtnl, skb, pid, gfp_any());
+   return nlmsg_unicast(rtnl, skb, pid, flags);
 }
 EXPORT_SYMBOL(rtnl_unicast);
 
@@ -2565,7 +2565,8 @@ static int rtnl_getlink(struct sk_buff *skb, struct 
nlmsghdr* nlh)
WARN_ON(err == -EMSGSIZE);
kfree_skb(nskb);
} else
-   err = rtnl_unicast(nskb, net, NETLINK_CB(skb).portid);
+   err = rtnl_unicast(nskb, net, NETLINK_CB(skb).portid,
+  GFP_KERNEL);
 
return err;
 }
@@ -3601,7 +3602,8 @@ static int rtnl_stats_get(struct sk_buff *skb, struct 
nlmsghdr *nlh)
WARN_ON(err == -EMSGSIZE);
kfree_skb(nskb);
} else {
-   err = rtnl_unicast(nskb, net, NETLINK_CB(skb).portid);
+   err = rtnl_unicast(nskb, net, NETLINK_CB(skb).portid,
+  GFP_KERNEL);
}
 
return err;
diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c
index 4f6c186..e4de9fe 100644
--- a/net/dcb/dcbnl.c
+++ b/net/dcb/dcbnl.c
@@ -1749,7 +1749,7 @@ static int dcb_doit(struct sk_buff *skb, struct nlmsghdr 
*nlh)
 
nlmsg_end(reply_skb, reply_nlh);
 
-   ret = rtnl_unicast(reply_skb, net, portid);
+   ret = rtnl_unicast(reply_skb, net, portid, GFP_KERNEL);
 out:
return ret;
 }
diff --git a/net/decnet/dn_route.c b/net/decnet/dn_route.c
index b1dc096..6fe02bb 100644
--- a/net/decnet/dn_route.c
+++ b/net/decnet/dn_route.c
@@ -1714,7 +1714,8 @@ static int dn_cache_getroute(struct sk_buff *in_skb, 
struct nlmsghdr *nlh)
goto out_free;
}
 
-   return rtnl_unicast(skb, _net, NETLINK_CB(in_skb).portid);
+   return rtnl_unicast(skb, _net, NETLINK_CB(in_skb).portid,
+   GFP_KERNEL);
 
 out_free:
kfree_skb(skb);
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index e333bc8..5e969e5 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1917,7 +1917,7 @@ static int inet_netconf_get_devconf(struct sk_buff 
*in_skb,
kfree_skb(skb);
goto errout;
}
-   err = rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid);
+   err = rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid, GFP_ATOMIC);
 errout:
return err;
 }
diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 5ad48ec..c704a2a 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -654,7 +654,8 @@ static void ipmr_destroy_unres(struct mr_table *mrt, struct 
mfc_cache *c)
e->error = -ETIMEDOUT;
memset(>msg, 0, sizeof(e->msg));
 
-   rtnl_unicast(skb, net, NETLINK_CB(skb).portid);
+   rtnl_unicast(skb, net, NETLINK_CB(skb).portid,
+