lkml.org 
[lkml]   [2009]   [Nov]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [net-next-2.6 PATCH RFC] TCPCT part 1d: generate Responder Cookie
On Thu, Nov 05, 2009 at 07:17:42AM -0500, William Allen Simpson wrote:
> Paul E. McKenney wrote:
>> On Tue, Nov 03, 2009 at 05:38:10PM -0500, William Allen Simpson wrote:
>>> Documentation/RCU/checklist.txt #7 says:
>>>
>>> One exception to this rule: rcu_read_lock() and rcu_read_unlock()
>>> may be substituted for rcu_read_lock_bh() and rcu_read_unlock_bh()
>>> in cases where local bottom halves are already known to be
>>> disabled, for example, in irq or softirq context. Commenting
>>> such cases is a must, of course! And the jury is still out on
>>> whether the increased speed is worth it.
>> I strongly suggest using the matching primitives unless you have a
>> really strong reason not to.
> Eric gave contrary advice. But he also suggested (in an earlier message)
> clearing the secrets with a timer, which could be a separate context --
> although much later in time.
>
> As you suggest, I'll use the _bh suffix everywhere until every i is dotted
> and t is crossed. Then, check for efficiency later after thorough
> analysis by experts such as yourself.
>
> This code will be hit on every SYN and SYNACK that has a cookie option.
> But it's just prior to a CPU intensive sha_transform -- in comparison,
> it's trivial.

Had Eric said that this code were performance-critical, where every
nanosecond mattered, that would certainly be good enough for me.
Eric has excellent knowledge of the networking code, certainly much
better than mine. And 10Gb Ethernet is certainly a performance
challenge, and I don't expect 40Gb Ethernet to be any easier.

Of course, I would still argue that the use of rcu_read_lock() rather
than rcu_read_unlock() needs to be commented. And if this sort of
substitution happens a lot, maybe we need a way for it to happen
automatically.

Thanx, Paul

>>> + rcu_assign_pointer(tcp_secret_generating,
>>> + tcp_secret_secondary);
>>> + rcu_assign_pointer(tcp_secret_retiring,
>>> + tcp_secret_primary);
>>> + spin_unlock_bh(&tcp_secret_locker);
>>> + /* call_rcu() or synchronize_rcu() not needed. */
>> Would you be willing to say why? Are you relying on a time delay for a
>> given item to pass through tcp_secret_secondary and tcp_secret_retiring
>> or some such? If so, how do you know that this time delay will always
>> be long enough?
>> Or are you just shuffling the data structures around, without ever
>> freeing them? If so, is it really OK for a given reader to keep a
>> reference to a given item through the full range of shuffling, especially
>> given that it might be accesssing this concurrently with the ->expires
>> assignments above?
>> Either way, could you please expand the comment to give at least some
>> hint to the poor guy reading your code? ;-)
> Yes. Just shuffling the pointers without ever freeing anything. So,
> there's nothing for call_rcu() to do, and nothing else to synchronize
> (only the pointers). This assumes that after _unlock_ any CPU cache
> with an old pointer->expires will hit the _lock_ code, and that will
> update *both* ->expires and the other array elements concurrently?
>
> One of the advantages of this scheme is the new secret is initialized
> while the old secret is still used, and the old secret can continue to
> be verified as old packets arrive. (I originally designed this for
> Photuris [RFC-2522] circa 1995.)
>
> As described in the long header given, each array element goes through
> four (4) states. This is handling the first state transition. It will
> hit at least 2 more locks, pointer updates, and unlocks before reuse.
>
> Also, a great deal of time passes. After being retired (and expired), it
> will be unused for approximately 5 minutes.
>
> All that's a bit long for a comment.
>
> + /*
> + * The retiring data is never freed. Instead, it is
> + * replaced after later pointer updates and a quiet
> + * time of approximately 5 minutes. There is nothing
> + * for call_rcu() or synchronize_rcu() to handle.
> + */
>
> Clear enough?


\
 
 \ /
  Last update: 2009-11-05 16:01    [W:0.052 / U:0.388 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site