[patch] reducing arp locking

[Alexander V. Chernikov]

On 09.11.2012 13:59, Fabien Thomas wrote:
>
> Le 9 nov. 2012 à 10:05, Alexander V. Chernikov a écrit :
>
>> On 09.11.2012 12:51, Fabien Thomas wrote:
>>>
>>> Le 8 nov. 2012 à 11:25, Alexander V. Chernikov a écrit :
>>>
>>>> On 08.11.2012 14:24, Andre Oppermann wrote:
>>>>> On 08.11.2012 00:24, Alexander V. Chernikov wrote:
>>>>>> Hello list!
>>>>>>
>>>>>> Currently we need to acquire 2 read locks to perform simple 6-byte
>>>>>> copying from arp record to packet
>>>>>> ethernet header.
>>>>>>
>>>>>> It seems that acquiring lle lock for fast path (main traffic flow) is
>>>>>> not necessary even with
>>>>>> current code.
>>>>>>
>>>>>> My tests shows ~10% improvement with this patch applied.
>>>>>>
>>>>>> If nobody objects I plan to commit this change at the end of next week.
>>>>>
>>>>> This is risky and prone to race conditions.  The copy of the MAC address
>>>>> should be done while the table read lock is held to protect against the
>>>> It is done exactly as you say: table read lock is held.
>>>
>>> How do you protect from entry update if i've a ref to the entry ?
>>> You can end up doing bcopy of a partial mac address.
>> I see no problems in copying incorrect mac address in that case:
>> if host mac address id updated, this is, most likely, another host, and several packets being lost changes nothing.
>
> Sending packet to a bogus mac address is not really nothing :)
>
>>
>> However, there can be some realistic scenario where this can be the case (L2 load balancing/failover). I'll update in_arpinput() to do lle removal/insertion in that case.
>>
>>> la_preempt modification is also write access to an unlocked structure.
>> This one changes nothing:
>> current code does this under _read_ lock.
>
> Under the table lock not the entry lock ?
lle entry is read-locked while la_preempt is modified.

> Table lock is here to protect the table if I've understood the code correctly.
Yes.
> If i get an exclusive reference to the entry you will end up reading and writing to the entry without any lock.
Yes. And the only single drawback in worst case can be sending a bit 
more packets to right (but probably expired) MAC address.

I'm talking about the following:
ARP stack is just IP -> 6 bytes mapping, there is no reason to make it 
unnecessary complicated like rte, with references being held by upper 
layer stack. It does not contain interface pointer, etc..

We may need to r/w lock entry, but for 'control plane' code only.
If one acquired exclusive lock and wants to change STATIC flag to 
non-static or change lle address - this is simply wrong and has to be 
handled by acquiring table wlock.

Current ARP code has some flaws like handling arp expiration, but this 
patch doesn't change much here..

>
>>
>>>
>>>
>>>>
>>>>> entry going away.  You can either return with table lock held and drop
>>>>> it after the copy, or you could a modified lookup function that takes a
>>>>> pointer for the copy destination, do the copy with the read lock, and then
>>>>> return.  If no entry is found an error is returned and obviously no copy
>>>>> is done.
>>>>>
>>>>
>>>>
>>>> --
>>>> WBR, Alexander
>>>>
>>>>
>>>> _______________________________________________
>>>> freebsd-hackers at freebsd.org mailing list
>>>> http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
>>>> To unsubscribe, send any mail to "freebsd-hackers-unsubscribe at freebsd.org"
>>>
>>>
>>
>
>


-- 
WBR, Alexander