Messages in this thread | | | From | "Jason A. Donenfeld" <> | Date | Mon, 25 Mar 2019 11:13:59 +0100 | Subject | Re: [PATCH net-next v9 19/19] net: WireGuard secure network tunnel |
| |
Hey Dave,
Thanks for the comments.
On Mon, Mar 25, 2019 at 1:03 AM David Miller <davem@davemloft.net> wrote: > Said another way: "The code isn't optimal with my compiler on my computer > unless I force inline this" is not a valid reason to use __always_inline
Hah, that was my rationale exactly. Okay, no problem, I'll remove those. And if I can't find a way to make the compiler to do the right thing without it, and nothing looks overly insane, then I'll fix gcc bugs separately.
> > + ((u64 *)dst)[0] = be64_to_cpu(((const __be64 *)src)[0]); > > + ((u64 *)dst)[1] = be64_to_cpu(((const __be64 *)src)[1]); > > Are 'dst' and 'src' both 64-bit aligned? If not you'll get traps on some cpus.
Yes, they are.
> > > + __skb_queue_head_init(&packets); > > + if (!skb_is_gso(skb)) { > > + skb->next = NULL; > > Why? Direct ->next and ->prev pointer accesses should never be used, > along with anything that assumes what the implentation of skb lists > looks like. > > Always use the helpers instead.
Ack, will do.
> > --- /dev/null > > +++ b/drivers/net/wireguard/hashtables.c > > No way. > > Do not invent your own hashtables, we have several generic versions in > tree and in particular rhashtable. > > If the generic kernel facilities have a weakness, fix that instead of > rolling an entire new hashtable implementation.
That file might be poorly named; I'm certainly not implementing my own hashtable, but rather using <linux/hashtable.h>. The file simply has the various accessors for it that does the proper locking and reference counting associated with the things inserted into the vanilla <linux/hashtable.h> hashtable. I'll rename the file to something more descriptive.
Jason
| |