Re: Improving use of rt_refcnt

[Ryota Ozaki <ozaki-r%netbsd.org@localhost>]

On Mon, Jul 6, 2015 at 2:33 PM, Dennis Ferguson
<dennis.c.ferguson%gmail.com@localhost> wrote:
>
> On 5 Jul, 2015, at 19:02 , Ryota Ozaki <ozaki-r%netbsd.org@localhost> wrote:
>> On Sun, Jul 5, 2015 at 6:50 PM, Joerg Sonnenberger
>> <joerg%britannica.bec.de@localhost> wrote:
>>> On Sun, Jul 05, 2015 at 02:12:18PM +0900, Ryota Ozaki wrote:
>>>> On Sun, Jul 5, 2015 at 2:35 AM, David Young <dyoung%pobox.com@localhost> wrote:
>>>>> On Sat, Jul 04, 2015 at 09:52:56PM +0900, Ryota Ozaki wrote:
>>>>>> I'm trying to improve use of rt_refcnt: reducing
>>>>>> abuse of it, e.g., rt_refcnt++/rt_refcnt-- outside
>>>>>> route.c and extending it to treat referencing
>>>>>> during packet processing (IOW, references from
>>>>>> local variables) -- currently it handles only
>>>>>> references between routes. The latter is needed for
>>>>>> MP-safe networking.
>>>>>
>>>>> Do you propose to increase/decrease rt_refcnt in the packet processing
>>>>> path, using atomic instructions?
>>>>
>>>> Atomic instructions aren't used yet, i.e., softnet_lock is still needed.
>>>> I will introduce them later. (Using refcount(9) by riastradh would be
>>>> good once it is committed.)
>>>
>>> I think the main point that David wanted to raise is that the normal
>>> path for packets should *not* do any ref count changes at all.
>>
>> Why? rtentry can be freed during the normal path in MP-safe world.
>> Do you suggest using pserialize instead?
>
> I don't think either a reference count or pserialize, or anything else
> that is non-blocking for readers, can be used to protect the data
> structure rtentry's are now stored in.

I'm sorry for confusing you, our first attempt doesn't intend to provide
non-blocking reader operations. But yet I misunderstood about
the characteristic of the routing table you described below.

>
> If you want readers to continue while a data structure is being modified
> then the modification must be implemented so that concurrent readers see
> the structure in a consistent state (i.e. one that produces either the
> before-the-change or the after-the-change result) at every point during
> the change.  Since the current radix tree does not work this way the only
> way to make a change to it is to block the readers while the change is
> being made, i.e. with a lock.  An rtentry will hence never be freed while
> the normal (reader) path is looking at it since you'll be preventing those
> readers from looking at anything in that structure while you are changing it.

I got what you mean. The current implementation just doesn't free a rtentry
if there are references to it but does modify a rtentry regardless of refcnt
of it. I'll use a lock somehow to prevent the latter. Nonetheless, I think
my patch is still useful to prevent the former. (And anyway we have to reduce
awkward use of refcnt.)

>
> If you don't want it to work this way then you'll need to replace the
> radix tree with something that permits changes while readers are
> concurrently operating.

IIUC, your rttree(9) satisfies the requirement, right? We're evaluating
rttree(9) as a replacement of the current radix tree. Do you have any
updates on rttree(9)?

  ozaki-r

> To take best advantage of a more modern data
> structure, however, you are still not going to want readers to ever
> write the shared data structure if that can be avoided.  The two
> atomic operations needed to increment and decrement a reference count
> greatly exceed the cost of a (well-cached) route lookup.
>
> Dennis Ferguson