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.
