On 19.12.2016 17:40, Eric Dumazet wrote:
> On Mon, 2016-12-19 at 17:36 +0100, Hannes Frederic Sowa wrote:
>> On 19.12.2016 17:17, Eric Dumazet wrote:
>>> On Sun, 2016-12-18 at 22:56 +0200, Julian Anastasov wrote:
>>>
>>>>
>>>> +static inline void sock_confirm_neigh(struct sk_buff *skb, struct
>>>> neighbour *n)
>>>> +{
>>>> + if (unlikely(skb->dst_pending_confirm)) {
>>>> + struct sock *sk = skb->sk;
>>>> + unsigned long now = jiffies;
>>>> +
>>>> + /* avoid dirtying neighbour */
>>>> + if (n->confirmed != now)
>>>> + n->confirmed = now;
>>>> + if (sk && sk->sk_dst_pending_confirm)
>>>> + sk->sk_dst_pending_confirm = 0;
>>>> + }
>>>> +}
>>>> +
>>>
>>> I am still digesting this awesome patch series ;)
>>>
>>> Not sure why you used an unlikely() here. TCP for example would hit this
>>> path quite often.
>>>
>>> So considering sk_dst_pending_confirm might be dirtied quite often,
>>>
>>> I am not sure why you placed it in the cache line that contains
>>> sk_rx_dst (in 1st patch)
>>
>> Because they have to stay synchronized?
>>
>> If we modify sk_rx_dst, we automatically also must clear
>> pending_confirm, otherwise we might end up confirming a wrong neighbor.
>
> Your answer makes little sense really...
>
> For most TCP flows, we set sk_rx_dst exactly once.
>
> Hardly a good reason to have these in the same cache line.
Right :) , and I didn't actually wanted to make an argument in favor of
that. Just noted they are probably semantically grouped together as an
explanation.
Sorry,
Hannes
