Re: [PATCH net-next 1/4] geneve: implement geneve_get_sk_family helper

[Jiri Benc <jbenc@redhat.com>]

On Thu, 18 Feb 2016 09:44:10 -0500, John W. Linville wrote:
> On Thu, Feb 18, 2016 at 11:22:49AM +0100, Jiri Benc wrote:
> > +static sa_family_t geneve_get_sk_family(struct geneve_sock *gs)
> > +{
> > +	return gs->sock->sk->sk_family;
> > +}
> > +
> 
> Should this be inline?

AFAIK the kernel policy is to not add inlines in .c files, instead let
the compiler do its job.

> Can we count on GCC to inline geneve_get_sk_family on its own?

Yes.

> Otherwise, we are calling the same function as many as three times
> on receive.
[...]
> Then on these three you are caching the results of the function
> call instead...

The reason I removed the sa_family local variable from geneve_rx is the
next patch in the set where I need to put part of the family references
into a separate function.

I kept the rest of the file as it was - I thought about removing the
local variables in geneve_notify_add/del_rx_port, too, but it would
lead to ugly long lines here:

        for_each_netdev_rcu(net, dev) {
                if (dev->netdev_ops->ndo_add_geneve_port)
                        dev->netdev_ops->ndo_add_geneve_port(dev, sa_family,
                                                             port);
        }

Thus, the local variable makes sense in the these two functions, makes
the code more comprehensible.

> > @@ -596,7 +598,7 @@ static struct geneve_sock *geneve_find_sock(struct geneve_net *gn,
> >  
> >  	list_for_each_entry(gs, &gn->sock_list, list) {
> >  		if (inet_sk(gs->sock->sk)->inet_sport == dst_port &&
> > -		    inet_sk(gs->sock->sk)->sk.sk_family == family) {
> > +		    geneve_get_sk_family(gs) == family) {
> >  			return gs;
> >  		}
> >  	}
> 
> And back to calling it each time (as necessary inside the
> list_for_each_entry).

Just replacing the direct sk_family dereference with the helper, no
real change here. No need to introduce a local variable here, this is
still short enough.

> So, it seems like geneve_get_sk_family needs to be inline -- maybe GCC
> is doing that on its own?  FWIW, I probably would cache the results
> inside of geneve_rx like you have done in geneve_notify_add_rx_port
> and geneve_notify_del_rx_port.

Then I would just have to remove or duplicate the local variable in the
next patch. Not worth it, I think.

Thanks for review,

 Jiri