[patch] reducing arp locking

[Fabien Thomas]

Le 9 nov. 2012 à 12:18, Alexander V. Chernikov a écrit :

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

Or partial copy of the new mac.
> 
> 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..

In in_arpinput only exclusive access to the entry is taken during the update no IF_AFDATA_LOCK that's why i was surprised.


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