netdev - Re: [PATCH 2/4] [CORE]: datagram: mem

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <47659682.4030204@redhat.com>
Date:	Sun, 16 Dec 2007 16:20:02 -0500
From:	Hideo AOKI <haoki@...hat.com>
To:	Herbert Xu <herbert@...dor.apana.org.au>
CC:	David Miller <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>,
	Takahiro Yasui <tyasui@...hat.com>,
	Masami Hiramatsu <mhiramat@...hat.com>,
	Satoshi Oshima <satoshi.oshima.fk@...achi.com>,
	billfink@...dspring.com, Andi Kleen <andi@...stfloor.org>,
	Evgeniy Polyakov <johnpol@....mipt.ru>,
	Stephen Hemminger <shemminger@...ux-foundation.org>,
	yoshfuji@...ux-ipv6.org,
	Yumiko Sugita <yumiko.sugita.yf@...achi.com>
Subject: Re: [PATCH 2/4] [CORE]: datagram: mem_scheudle functions

Hello,

Thank you for your quick comments.

Herbert Xu wrote:
>> +	spin_lock_irqsave(&sk->sk_lock.slock, flags);
> 
> Please use bh_lock_sock since this must never be used from an
> IRQ handler.

I'll try to re-implement this locking mechanism as David suggested.

>> +static inline void sk_mem_reclaim(struct sock *sk)
>> +{
>> +	if (sk->sk_type == SOCK_DGRAM)
>> +		sk_datagram_mem_reclaim(sk);
>> +}
> 
> Please get rid of these wrappers since we should only get here
> for datagram protocols.

In my understanding, TCP also uses ip_append_data() and
ip_ufo_append_data() via ip_send_reply(). Then I thought TCP could
reach datagram memory accounting functions, and I used the wrappers
since I didn't want to change TCP's sk_alloc_forward and
memory_allocated.

I'll try to remove these wrappers in next patch set. But I'd
appreciate it if you let me know whether we can keep the wrappers
in case TCP memory accounting doesn't work well due to accounting
in IP layer.

>> +static inline int sk_account_wmem_charge(struct sock *sk, int size)
>> +{
>> +	unsigned long flags;
>> +
>> +	/* account if protocol supports memory accounting. */
>> +	if (!sk->sk_prot->memory_allocated || sk->sk_type != SOCK_DGRAM)
>> +		return 1;
>> +
>> +	spin_lock_irqsave(&sk->sk_lock.slock, flags);
>> +	if (sk_datagram_wmem_schedule(sk, size)) {
>> +		sk->sk_forward_alloc -= size;
>> +		spin_unlock_irqrestore(&sk->sk_lock.slock, flags);
>> +		return 1;
>> +	}
>> +	spin_unlock_irqrestore(&sk->sk_lock.slock, flags);
>> +	return 0;
>> +}
> 
> This is probably too big to inline.
> 
>> +static inline int sk_account_rmem_charge(struct sock *sk, int size)
>> +{
>> +	unsigned long flags;
>> +
>> +	/* account if protocol supports memory accounting. */
>> +	if (!sk->sk_prot->memory_allocated || sk->sk_type != SOCK_DGRAM)
>> +		return 1;
>> +
>> +	spin_lock_irqsave(&sk->sk_lock.slock, flags);
>> +	if (sk_datagram_rmem_schedule(sk, size)) {
>> +		sk->sk_forward_alloc -= size;
>> +		spin_unlock_irqrestore(&sk->sk_lock.slock, flags);
>> +		return 1;
>> +	}
>> +	spin_unlock_irqrestore(&sk->sk_lock.slock, flags);
>> +	return 0;
>> +}
> 
> Why are we duplicating the rmem/wmem versions when they're identical?

Good catch. I'll merge the body of these functions into one function.
Furthermore, I'll stop using inline if the code is large.

>> -int sock_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
>> +int sock_queue_rcv_skb_with_owner(struct sock *sk, struct sk_buff *skb,
>> +				  void set_owner_r(struct sk_buff *nskb,
>> +						   struct sock* nsk))
> 
> Just make a new function for this rather than playing with function
> pointers.

I understood. I'll fix it.

Again, many thanks for the review.

Regards,
Hideo

-- 
Hitachi Computer Products (America) Inc.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html