[Devel] [PATCH 7/7] packet: Pre-account maximum socket buffer into cg memory

[Pavel Emelyanov]

On 06/01/2015 07:01 PM, Vladimir Davydov wrote:
> On Fri, May 29, 2015 at 05:19:15PM +0300, Pavel Emelyanov wrote:
>> Packet sockets have incoming queue of packets that is only limited
>> with per-socket wmem buffer. Strictly speaking we should sum up
>> all the queues and charge them into kmem once new packet arrives,
>> but this will result in huge patch. Since there's typically quite
>> a few of packet sockets in container (tcpdump) we can just forward
>> charge the maximum socket rmem size into cg memory upon socket
>> creation.
> 
> rmem_max ~ 200K
> 
> Small? If you say so, OK.

Yes, 200K is quite small amount. In this context.

>> @@ -554,6 +554,20 @@ void sock_update_memcg(struct sock *sk)
>>  }
>>  EXPORT_SYMBOL(sock_update_memcg);
>>  
>> +struct mem_cgroup *sock_get_current_memcg(void)
> 
> I don't like this funciton, because it has nothing to do with sockets.
> May be, you'd better use try_get_mem_cgroup_from_mm?

>From what mm? From current->mm? I tried to find for try_get_mm_cgroup_from_task
but failed, this is the one I need :D

>> +{
>> +	struct mem_cgroup *cg;
>> +
>> +	rcu_read_lock();
>> +	cg = mem_cgroup_from_task(current);
>> +	if (mem_cgroup_is_root(cg) || !css_tryget(&cg->css))
>> +		cg = NULL;
>> +	rcu_read_unlock();
>> +
>> +	return cg;
>> +}
>> +EXPORT_SYMBOL(sock_get_current_memcg);
>> +
>>  void sock_release_memcg(struct sock *sk)
>>  {
>>  	if (mem_cgroup_sockets_enabled && sk->sk_cgrp) {
>> @@ -564,6 +578,12 @@ void sock_release_memcg(struct sock *sk)
>>  	}
>>  }
>>  
>> +void sock_put_memcg(struct mem_cgroup *cg)
>> +{
>> +	css_put(&cg->css);
>> +}
>> +EXPORT_SYMBOL(sock_put_memcg);
>> +
> 
> I would use css_put(mem_cgroup_css(memcg)) instead. Awkward? Perhaps,
> but memcg users do that, and you wouldn't have to introduce yet another
> function.

OK, but I will introduce it anyway with the body you suggest :)

>>  struct cg_proto *tcp_proto_cgroup(struct mem_cgroup *memcg)
>>  {
>>  	if (!memcg || mem_cgroup_is_root(memcg))
>> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
>> index e8b5a0d..5a13dfa 100644
>> --- a/net/packet/af_packet.c
>> +++ b/net/packet/af_packet.c
>> @@ -2420,6 +2420,67 @@ static int packet_sendmsg(struct kiocb *iocb, struct socket *sock,
>>  		return packet_snd(sock, msg, len);
>>  }
>>  
>> +#ifdef CONFIG_MEMCG_KMEM
>> +struct packet_sk_charge {
>> +	struct mem_cgroup	*memcg;
>> +	unsigned long		amt;
>> +};
>> +
>> +static struct cg_proto *packet_sk_charge(void)
>> +{
>> +	struct packet_sk_charge *psc;
>> +
>> +	if (!mem_cgroup_sockets_enabled)
>> +		return NULL;
>> +
>> +	psc = kmalloc(sizeof(*psc), GFP_KERNEL);
>> +	if (!psc)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	psc->memcg = sock_get_current_memcg();
>> +	if (psc->memcg) {
>> +		/*
>> +		 * Forcedly charge the maximum amount of data this socket
>> +		 * may have. It's typically not huge and packet sockets are
>> +		 * rare guests in containers, so we don't disturb the memory
>> +		 * consumption much.
>> +		 */
>> +		psc->amt = sysctl_rmem_max;
> 
> If psc->amt is constant, may be it'd better not to introduce struct
> packet_sk_charge, but return memcg instead?

No, this constant can be different at uncharge time.

>> +		memcg_charge_kmem_bytes(psc->memcg, psc->amt);
>> +	} else {
>> +		kfree(psc);
>> +		psc = NULL;
>> +	}
>> +	rcu_read_unlock();
> 
> Stale?

Yes :(

>> +
>> +	/*
>> +	 * The sk->sk_cgrp is not used for packet sockets,
>> +	 * so we'll just put the smaller structure into it.
>> +	 */
>> +	return (struct cg_proto *)psc;
>> +}
>> +
>> +static void packet_sk_uncharge(struct cg_proto *cg)
>> +{
>> +	struct packet_sk_charge *psc = (struct packet_sk_charge *)cg;
>> +
>> +	if (psc) {
>> +		memcg_uncharge_kmem_bytes(psc->memcg, psc->amt);
>> +		sock_put_memcg(psc->memcg);
>> +		kfree(psc);
>> +	}
>> +}
>> +#else
>> +static struct cg_proto *packet_sk_charge(void)
>> +{
>> +	return NULL;
>> +}
>> +
>> +static void packet_sk_uncharge(struct sock *sk)
> 
> It does not match the definition above.

:D

-- Pavel