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-hack...@freebsd.org mailing list >>>>> http://lists.freebsd.org/mailman/listinfo/freebsd-hackers >>>>> To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org" >>>> >>>> >>> >> >> > > > -- > WBR, Alexander > > _______________________________________________ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
