Re: [PATCH 5/7] netpoll: Move SKBs pool to netconsole side

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Breno Leitao wrote:
> Since netconsole is the sole user of the SKBs pool within netpoll, move
> the pool management into the netconsole driver.
> 
> This change prevents other netpoll users from allocating and holding
> onto skb pool memory unnecessarily, thereby reducing memory usage when
> the pool is not required (which is all the cases except netconsole).
> 
> The skb poll struct is still attached to the netpoll, but, eventually
> this should move to the netconsole target, since it has nothing to do
> with netpoll.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  drivers/net/netconsole.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++--
>  net/core/netpoll.c       | 44 ------------------------------------
>  2 files changed, 56 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 90e359b87469a..3fe55db07cfe5 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -57,6 +57,19 @@ MODULE_LICENSE("GPL");
>  #define MAX_EXTRADATA_ITEMS		16
>  #define MAX_PRINT_CHUNK			1000
>  
> +/*
> + * We maintain a small pool of fully-sized skbs, to make sure the
> + * message gets out even in extreme OOM situations.
> + */
> +
> +#define MAX_SKBS 32
> +#define MAX_UDP_CHUNK 1460
> +#define MAX_SKB_SIZE							\
> +	(sizeof(struct ethhdr) +					\
> +	 sizeof(struct iphdr) +						\
> +	 sizeof(struct udphdr) +					\
> +	 MAX_UDP_CHUNK)
> +
>  static char config[MAX_PARAM_LENGTH];
>  module_param_string(netconsole, config, MAX_PARAM_LENGTH, 0);
>  MODULE_PARM_DESC(netconsole, " netconsole=[src-port]@[src-ip]/[dev],[tgt-port]@<tgt-ip>/[tgt-macaddr]");
> @@ -172,6 +185,33 @@ struct netconsole_target {
>  	char			buf[MAX_PRINT_CHUNK];
>  };
>  
> +static void refill_skbs(struct netpoll *np)
> +{
> +	struct sk_buff_head *skb_pool;
> +	struct sk_buff *skb;
> +	unsigned long flags;
> +
> +	skb_pool = &np->skb_pool;
> +
> +	spin_lock_irqsave(&skb_pool->lock, flags);
> +	while (skb_pool->qlen < MAX_SKBS) {
> +		skb = alloc_skb(MAX_SKB_SIZE, GFP_ATOMIC);
> +		if (!skb)
> +			break;
> +
> +		__skb_queue_tail(skb_pool, skb);
> +	}
> +	spin_unlock_irqrestore(&skb_pool->lock, flags);
> +}
> +
> +static void refill_skbs_work_handler(struct work_struct *work)
> +{
> +	struct netpoll *np =
> +		container_of(work, struct netpoll, refill_wq);
> +
> +	refill_skbs(np);
> +}
> +
>  #ifdef	CONFIG_NETCONSOLE_DYNAMIC
>  
>  static struct configfs_subsystem netconsole_subsys;
> @@ -341,6 +381,20 @@ static int netpoll_parse_ip_addr(const char *str, union inet_addr *addr)
>  	return -1;
>  }
>  
> +static int setup_netpoll(struct netpoll *np)

Having both netpoll_setup and setup_netpoll is a bit confusing.
Maybe netconsole_setup_netpoll?

> +{
> +	int err;
> +
> +	err = netpoll_setup(np);
> +	if (err)
> +		return err;
> +
> +	refill_skbs(np);
> +	INIT_WORK(&np->refill_wq, refill_skbs_work_handler);
> +
> +	return 0;
> +}
> +
>  #ifdef	CONFIG_NETCONSOLE_DYNAMIC
>  
>  /*
> @@ -615,7 +669,7 @@ static ssize_t enabled_store(struct config_item *item,
>  		 */
>  		netconsole_print_banner(&nt->np);
>  
> -		ret = netpoll_setup(&nt->np);
> +		ret = setup_netpoll(&nt->np);
>  		if (ret)
>  			goto out_unlock;
>  
> @@ -2036,7 +2090,7 @@ static struct netconsole_target *alloc_param_target(char *target_config,
>  	if (err)
>  		goto fail;
>  
> -	err = netpoll_setup(&nt->np);
> +	err = setup_netpoll(&nt->np);
>  	if (err) {
>  		pr_err("Not enabling netconsole for %s%d. Netpoll setup failed\n",
>  		       NETCONSOLE_PARAM_TARGET_PREFIX, cmdline_count);