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  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]
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

Powered by blists - more mailing lists