| Message ID | 1399906332.31472.1.camel@tkhai |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Kirill Tkhai <ktkhai@parallels.com> Date: Mon, 12 May 2014 18:52:12 +0400 > data_len = min_t(size_t, > len - SKB_MAX_ALLOC, > MAX_SKB_FRAGS * PAGE_SIZE); > + data_len = min_t(size_t, > + len, > + PAGE_ALIGN(data_len)); > + } When I see: x = min(y - N, z1); x = min(y, z2); I'm a bit suspicious. Why are you not subtracting the constant factor out of the first variable in the second min() call? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
В Ср, 14/05/2014 в 15:15 -0400, David Miller пишет: > From: Kirill Tkhai <ktkhai@parallels.com> > Date: Mon, 12 May 2014 18:52:12 +0400 > > > data_len = min_t(size_t, > > len - SKB_MAX_ALLOC, > > MAX_SKB_FRAGS * PAGE_SIZE); > > + data_len = min_t(size_t, > > + len, > > + PAGE_ALIGN(data_len)); > > + } > > When I see: > > x = min(y - N, z1); > x = min(y, z2); > > I'm a bit suspicious. Why are you not subtracting the constant > factor out of the first variable in the second min() call? Because, in the most cases (len - SKB_MAX_ALLOC) < PAGE_ALIGN(data_len), and the only payload of the patch is it tries to fix that :) We use unused space of allocated page. For mem cache it's easier to allocate (len - PAGE_ALIGN(x)) than (len - x). Here really should be data_len = min_t(size_t, len - SKB_MAX_ALLOC, MAX_SKB_FRAGS * PAGE_SIZE); + data_len = PAGE_ALIGN(data_len)); I added the second min, because I was afraid somebody plays with SKB_MAX_ALLOC size in debug purposes. Not sure now. Ok, yes, simple PAGE_ALIGN is much better :) + data_len = PAGE_ALIGN(data_len)); And I assumed it's OK to allocate skbs with zero header size like this: sock_alloc_send_pskb(sk, 0, data_len) Please say, if it's wrong. Thanks! Kirill -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-05-15 at 12:54 +0400, Kirill Tkhai wrote: > > And I assumed it's OK to allocate skbs with zero header size like this: > > sock_alloc_send_pskb(sk, 0, data_len) > > Please say, if it's wrong. > Its OK -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 749f80c..0830005 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -1492,10 +1492,14 @@ static int unix_dgram_sendmsg(struct kiocb *kiocb, struct socket *sock, if (len > sk->sk_sndbuf - 32) goto out; - if (len > SKB_MAX_ALLOC) + if (len > SKB_MAX_ALLOC) { data_len = min_t(size_t, len - SKB_MAX_ALLOC, MAX_SKB_FRAGS * PAGE_SIZE); + data_len = min_t(size_t, + len, + PAGE_ALIGN(data_len)); + } skb = sock_alloc_send_pskb(sk, len - data_len, data_len, msg->msg_flags & MSG_DONTWAIT, &err, @@ -1670,6 +1674,8 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock, data_len = max_t(int, 0, size - SKB_MAX_HEAD(0)); + data_len = min_t(size_t, size, PAGE_ALIGN(data_len)); + skb = sock_alloc_send_pskb(sk, size - data_len, data_len, msg->msg_flags & MSG_DONTWAIT, &err, get_order(UNIX_SKB_FRAGS_SZ));
Using whole of allocated pages reduces requested skb->data size. This is just a more thriftily allocation. netperf does not show difference with the current performance. Signed-off-by: Kirill Tkhai <ktkhai@parallels.com> --- net/unix/af_unix.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html