Re: MP-safe ifnet with psz & psref

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

Hi,

So I prepared patches:
  http://www.netbsd.org/~ozaki-r/m_set_rcvif.diff
  http://www.netbsd.org/~ozaki-r/m_get_rcvif.diff

The first patch introduces m_set_rcvif and m_reset_rcvif
that provide consistency API against m_get_rcvif,
hide internal of rcvif operation, and reduce the diff
of the second patch. No functional change.

The second patch realizes the proposal; store if_index
into mbuf instead of a pointer of an ifnet object and
look up an ifnet object by if_index when we get it from
a mbuf (*). The patch provides two APIs:
m_{get,put}_rcvif_psref that use psref(9) for sleep-able
critical sections and m_{get,put}_rcvif that use
pserialize(9) for other critical sections. The patch
also another API called m_get_rcvif_NOMPSAFE, that is NOT
MP-safe and for transition moratorium, i.e., it is intended
to be used for places where are not planned to be MP-ified
soon.

(*) In reality we need to preserve a pointer (void *)
    for M_{SET,GET}CTX so that we keep it as a union
    with if_index.

I measured the overhead of introducing psref in fast paths,
bridge forwarding and IP forwarding, and confirmed that
the overhead is negligible, 2% down at worst.

Thanks,
  ozaki-r

On Sun, May 22, 2016 at 6:10 PM, Ryota Ozaki <ozaki-r%netbsd.org@localhost> wrote:
> On Fri, May 20, 2016 at 6:11 PM, Ryota Ozaki <ozaki-r%netbsd.org@localhost> wrote:
>> On Fri, May 20, 2016 at 5:38 PM, Joerg Sonnenberger <joerg%bec.de@localhost> wrote:
>>> On Fri, May 20, 2016 at 12:15:10PM +0900, Ryota Ozaki wrote:
>>>> > Mbufs are slightly different. They can be queued on an interface for a
>>>> > moderately long time. For this purpose, a "flush" hook would definitely
>>>> > be needed. I think having such a hook would be useful for other purposes
>>>> > as well, i.e. to trigger memory compaction. But it is a somewhat
>>>> > expensive change in that it requires review of a lot of code.
>>>> >
>>>> > The final question for me is whether we can get away with just not
>>>> > deallocating ifnet instances. Reusing them is fine in principle, the
>>>> > problem is just keeping the memory type safe.
>>>>
>>>> What "keeping the memory type safe" means?
>>>
>>> Once allocated as ifnet, it will always be an ifnet.
>>
>> I see.
>>
>>>
>>>> The problem "when we can reuse it?" seems to be similar to "when we can
>>>> deallocate it?". Don't we need to do a similar solution I propose?
>>>
>>> The big question is what do we use rcvif for. If it is just a cookie
>>> used for preventing cycles, it doesn't matter what the content is.
>>
>> No, rcvif is used arbitrarily.
>>
>>> Otherwise we should have a field whether it was initialized completely
>>> or not.
>>
>> We can have IFF_INITIALIZED...oops, there is no space in if_flags :-<
>
> IIUC making such flag work requires some synchronization, a mutex or a psref,
> on an ifnet object to avoid race condition between clearing the initialized
> flag and proceeding destruction on a writer and checking the flag and
> referencing the object on a reader. Am I correct?
>
>   ozaki-r