Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1302751 > unrolled thread

[PATCH] net: add per device sg_max_frags for skb

Back to article view | Back to linux.kernel

Contents

  [PATCH] net: add per device sg_max_frags for skb Hans Westgaard Ry <hans.westgaard.ry@oracle.com> - 2016-01-06 14:20 +0100
    RE: [PATCH] net: add per device sg_max_frags for skb David Laight <David.Laight@ACULAB.COM> - 2016-01-06 15:10 +0100
      Re: [PATCH] net: add per device sg_max_frags for skb Hans Westgaard Ry <hans.westgaard.ry@oracle.com> - 2016-01-08 11:00 +0100
        RE: [PATCH] net: add per device sg_max_frags for skb David Laight <David.Laight@ACULAB.COM> - 2016-01-08 11:40 +0100
        Re: [PATCH] net: add per device sg_max_frags for skb Hannes Frederic Sowa <hannes@stressinduktion.org> - 2016-01-08 12:50 +0100
          Re: [PATCH] net: add per device sg_max_frags for skb Hans Westgaard Ry <hans.westgaard.ry@oracle.com> - 2016-01-13 15:00 +0100
            Re: [PATCH] net: add per device sg_max_frags for skb Eric Dumazet <edumazet@google.com> - 2016-01-13 15:30 +0100
            Re: [PATCH] net: add per device sg_max_frags for skb Eric Dumazet <edumazet@google.com> - 2016-01-13 15:30 +0100
              Re: [PATCH] net: add per device sg_max_frags for skb Hannes Frederic Sowa <hannes@stressinduktion.org> - 2016-01-13 16:10 +0100
              Re: [PATCH] net: add per device sg_max_frags for skb David Miller <davem@davemloft.net> - 2016-01-13 16:40 +0100
                Re: [PATCH] net: add per device sg_max_frags for skb Eric Dumazet <edumazet@google.com> - 2016-01-13 16:50 +0100
            Re: [PATCH] net: add per device sg_max_frags for skb ebiederm@xmission.com (Eric W. Biederman) - 2016-01-13 22:20 +0100
    Re: [PATCH] net: add per device sg_max_frags for skb Eric Dumazet <eric.dumazet@gmail.com> - 2016-01-06 15:10 +0100
      Re: [PATCH] net: add per device sg_max_frags for skb Hans Westgaard Ry <hans.westgaard.ry@oracle.com> - 2016-01-08 11:10 +0100

#1302751 — [PATCH] net: add per device sg_max_frags for skb

Devices may have limits on the number of fragments in an skb they
support. Current codebase uses a constant as maximum for number of
fragments (MAX_SKB_FRAGS) one skb can hold and use.

When enabling scatter/gather and running traffic with many small
messages the codebase uses the maximum number of fragments and thereby
violates the max for certain devices.

An example of such a violation is when running IPoIB on a HCA
supporting 16 SGE on an architecture with 4K pagesize. The
MAX_SKB_FRAGS will be 17 (64K/4K+1) and because IPoIB adds yet another
segment we end up with send_requests with 18 SGE resulting in
kernel-panic.

The patch allows the device to limit the maximum number fragments used
in one skb.

The functionality corresponds to gso_max_size/gso_max_segs for gso.

Signed-off-by: Hans Westgaard Ry <hans.westgaard.ry@oracle.com>
Reviewed-by: Håkon Bugge <haakon.bugge@oracle.com>
Reviewed-by: Knut Omang <knut.omang@oracle.com>
Reviewed-by: Wei Lin Guay <wei.lin.guay@oracle.com>
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@oracle.com>
Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>

---
 include/linux/netdevice.h | 8 ++++++++
 include/net/sock.h        | 2 ++
 net/core/dev.c            | 1 +
 net/core/sock.c           | 1 +
 net/ipv4/tcp.c            | 4 ++--
 5 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 3b5d134..c661865 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1513,6 +1513,8 @@ enum netdev_priv_flags {
  *			NIC for GSO
  *	@gso_min_segs:	Minimum number of segments that can be passed to the
  *			NIC for GSO
+ *     @sg_max_frags:  Maximum number of fragments that can be passed to the
+ *                     NIC for SG
  *
  *	@dcbnl_ops:	Data Center Bridging netlink ops
  *	@num_tc:	Number of traffic classes in the net device
@@ -1799,6 +1801,7 @@ struct net_device {
 	struct phy_device *phydev;
 	struct lock_class_key *qdisc_tx_busylock;
 	bool proto_down;
+	u16 sg_max_frags;
 };
 #define to_net_dev(d) container_of(d, struct net_device, dev)
 
@@ -3794,6 +3797,11 @@ static inline void netif_set_gso_max_size(struct net_device *dev,
 {
 	dev->gso_max_size = size;
 }
+static inline void netif_set_sg_max_frags(struct net_device *dev,
+					u16 max)
+{
+	dev->sg_max_frags = min_t(u16, MAX_SKB_FRAGS, max);
+}
 
 static inline void skb_gso_error_unwind(struct sk_buff *skb, __be16 protocol,
 					int pulled_hlen, u16 mac_offset,
diff --git a/include/net/sock.h b/include/net/sock.h
index 52d27ee..c884104 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -274,6 +274,7 @@ struct cg_proto;
   *	@sk_gso_type: GSO type (e.g. %SKB_GSO_TCPV4)
   *	@sk_gso_max_size: Maximum GSO segment size to build
   *	@sk_gso_max_segs: Maximum number of GSO segments
+  *    @sk_sg_max_frags: Maximum number of SG fragments
   *	@sk_lingertime: %SO_LINGER l_linger setting
   *	@sk_backlog: always used with the per-socket spinlock held
   *	@sk_callback_lock: used with the callbacks in the end of this struct
@@ -456,6 +457,7 @@ struct sock {
 	int			(*sk_backlog_rcv)(struct sock *sk,
 						  struct sk_buff *skb);
 	void                    (*sk_destruct)(struct sock *sk);
+	u16                     sk_sg_max_frags;
 };
 
 #define __sk_user_data(sk) ((*((void __rcu **)&(sk)->sk_user_data)))
diff --git a/net/core/dev.c b/net/core/dev.c
index ae00b89..abfbd3a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7106,6 +7106,7 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
 	dev->gso_max_size = GSO_MAX_SIZE;
 	dev->gso_max_segs = GSO_MAX_SEGS;
 	dev->gso_min_segs = 0;
+	dev->sg_max_frags = MAX_SKB_FRAGS;
 
 	INIT_LIST_HEAD(&dev->napi_list);
 	INIT_LIST_HEAD(&dev->unreg_list);
diff --git a/net/core/sock.c b/net/core/sock.c
index e31dfce..53d0cf0 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1621,6 +1621,7 @@ void sk_setup_caps(struct sock *sk, struct dst_entry *dst)
 		}
 	}
 	sk->sk_gso_max_segs = max_segs;
+	sk->sk_sg_max_frags = dst->dev->sg_max_frags;
 }
 EXPORT_SYMBOL_GPL(sk_setup_caps);
 
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index c82cca1..ca5f7a0 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -938,7 +938,7 @@ new_segment:
 
 		i = skb_shinfo(skb)->nr_frags;
 		can_coalesce = skb_can_coalesce(skb, i, page, offset);
-		if (!can_coalesce && i >= MAX_SKB_FRAGS) {
+		if (!can_coalesce && i >= sk->sk_sg_max_frags) {
 			tcp_mark_push(tp, skb);
 			goto new_segment;
 		}
@@ -1211,7 +1211,7 @@ new_segment:
 
 			if (!skb_can_coalesce(skb, i, pfrag->page,
 					      pfrag->offset)) {
-				if (i == MAX_SKB_FRAGS || !sg) {
+				if (i >= sk->sk_sg_max_frags || !sg) {
 					tcp_mark_push(tp, skb);
 					goto new_segment;
 				}
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1302784

RnJvbTogSGFucyBXZXN0Z2FhcmQgUnkNCj4gU2VudDogMDYgSmFudWFyeSAyMDE2IDEzOjE2DQo+
IERldmljZXMgbWF5IGhhdmUgbGltaXRzIG9uIHRoZSBudW1iZXIgb2YgZnJhZ21lbnRzIGluIGFu
IHNrYiB0aGV5DQo+IHN1cHBvcnQuIEN1cnJlbnQgY29kZWJhc2UgdXNlcyBhIGNvbnN0YW50IGFz
IG1heGltdW0gZm9yIG51bWJlciBvZg0KPiBmcmFnbWVudHMgKE1BWF9TS0JfRlJBR1MpIG9uZSBz
a2IgY2FuIGhvbGQgYW5kIHVzZS4NCj4gDQo+IFdoZW4gZW5hYmxpbmcgc2NhdHRlci9nYXRoZXIg
YW5kIHJ1bm5pbmcgdHJhZmZpYyB3aXRoIG1hbnkgc21hbGwNCj4gbWVzc2FnZXMgdGhlIGNvZGVi
YXNlIHVzZXMgdGhlIG1heGltdW0gbnVtYmVyIG9mIGZyYWdtZW50cyBhbmQgdGhlcmVieQ0KPiB2
aW9sYXRlcyB0aGUgbWF4IGZvciBjZXJ0YWluIGRldmljZXMuDQo+IA0KPiBBbiBleGFtcGxlIG9m
IHN1Y2ggYSB2aW9sYXRpb24gaXMgd2hlbiBydW5uaW5nIElQb0lCIG9uIGEgSENBDQo+IHN1cHBv
cnRpbmcgMTYgU0dFIG9uIGFuIGFyY2hpdGVjdHVyZSB3aXRoIDRLIHBhZ2VzaXplLiBUaGUNCj4g
TUFYX1NLQl9GUkFHUyB3aWxsIGJlIDE3ICg2NEsvNEsrMSkgYW5kIGJlY2F1c2UgSVBvSUIgYWRk
cyB5ZXQgYW5vdGhlcg0KPiBzZWdtZW50IHdlIGVuZCB1cCB3aXRoIHNlbmRfcmVxdWVzdHMgd2l0
aCAxOCBTR0UgcmVzdWx0aW5nIGluDQo+IGtlcm5lbC1wYW5pYy4NCj4gDQo+IFRoZSBwYXRjaCBh
bGxvd3MgdGhlIGRldmljZSB0byBsaW1pdCB0aGUgbWF4aW11bSBudW1iZXIgZnJhZ21lbnRzIHVz
ZWQNCj4gaW4gb25lIHNrYi4NCg0KVGhpcyBkb2Vzbid0IHNlZW0gdG8gbWUgdG8gYmUgdGhlIGNv
cnJlY3Qgd2F5IHRvIGZpeCB0aGlzLg0KQW55dGhpbmcgdGhhdCBhZGRzIGFuIGV4dHJhIGZyYWdt
ZW50IChpbiB0aGlzIGNhc2UgSVBvSUIpIHNob3VsZCBhbGxvdw0KZm9yIHRoZSBza2IgYWxyZWFk
eSBoYXZpbmcgdGhlIG1heGltdW0gbnVtYmVyIG9mIGZyYWdtZW50cy4NCkZ1bGx5IGxpbmVhcmlz
aW5nIHRoZSBza2IgaXMgb3ZlcmtpbGwsIGJ1dCBJIHRoaW5rIHRoZSBmaXJzdCBmcmFnbWVudA0K
Y2FuIGJlIGFkZGVkIHRvIHRoZSBsaW5lYXIgcGFydCBvZiB0aGUgc2tiLg0KDQoJRGF2aWQNCg0K
DQo=
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1304319

On 01/06/2016 02:59 PM, David Laight wrote:
> From: Hans Westgaard Ry
>> Sent: 06 January 2016 13:16
>> Devices may have limits on the number of fragments in an skb they
>> support. Current codebase uses a constant as maximum for number of
>> fragments (MAX_SKB_FRAGS) one skb can hold and use.
>>
>> When enabling scatter/gather and running traffic with many small
>> messages the codebase uses the maximum number of fragments and thereby
>> violates the max for certain devices.
>>
>> An example of such a violation is when running IPoIB on a HCA
>> supporting 16 SGE on an architecture with 4K pagesize. The
>> MAX_SKB_FRAGS will be 17 (64K/4K+1) and because IPoIB adds yet another
>> segment we end up with send_requests with 18 SGE resulting in
>> kernel-panic.
>>
>> The patch allows the device to limit the maximum number fragments used
>> in one skb.
> This doesn't seem to me to be the correct way to fix this.
> Anything that adds an extra fragment (in this case IPoIB) should allow
> for the skb already having the maximum number of fragments.
> Fully linearising the skb is overkill, but I think the first fragment
> can be added to the linear part of the skb.
>
> 	David
>
>
When IpoIB handles a skb-request it converts fragments to SGEs to
be handled by a HCA.
The problem arises when the HCA have a limited number of SGEs less than 
MAX_SKB_FRAGS.
(it gets a little worse since IPoIB need to yet another segment)
I have not found any easy way of fixing this with currenct codebase.

Hans

[toc] | [prev] | [next] | [standalone]

#1304356

From: Hans Westgaard
> Sent: 08 January 2016 09:56
...
> >> The patch allows the device to limit the maximum number fragments used
> >> in one skb.
> >
> > This doesn't seem to me to be the correct way to fix this.
> > Anything that adds an extra fragment (in this case IPoIB) should allow
> > for the skb already having the maximum number of fragments.
> > Fully linearising the skb is overkill, but I think the first fragment
> > can be added to the linear part of the skb.
> >
> > 	David
> >
> >
> When IpoIB handles a skb-request it converts fragments to SGEs to
> be handled by a HCA.
> The problem arises when the HCA have a limited number of SGEs less than
> MAX_SKB_FRAGS.
> (it gets a little worse since IPoIB need to yet another segment)
> I have not found any easy way of fixing this with currenct codebase.

I think one of the xen ethernet interfaces had a similar problem.

Just reduce the number of fragments by copying two (or more) of them
into a single fragment.
In effect, anything that reduces the number of fragments will do a copy.

	David

[toc] | [prev] | [next] | [standalone]

#1304439

On 08.01.2016 10:55, Hans Westgaard Ry wrote:
>
>
> On 01/06/2016 02:59 PM, David Laight wrote:
>> From: Hans Westgaard Ry
>>> Sent: 06 January 2016 13:16
>>> Devices may have limits on the number of fragments in an skb they
>>> support. Current codebase uses a constant as maximum for number of
>>> fragments (MAX_SKB_FRAGS) one skb can hold and use.
>>>
>>> When enabling scatter/gather and running traffic with many small
>>> messages the codebase uses the maximum number of fragments and thereby
>>> violates the max for certain devices.
>>>
>>> An example of such a violation is when running IPoIB on a HCA
>>> supporting 16 SGE on an architecture with 4K pagesize. The
>>> MAX_SKB_FRAGS will be 17 (64K/4K+1) and because IPoIB adds yet another
>>> segment we end up with send_requests with 18 SGE resulting in
>>> kernel-panic.
>>>
>>> The patch allows the device to limit the maximum number fragments used
>>> in one skb.
>> This doesn't seem to me to be the correct way to fix this.
>> Anything that adds an extra fragment (in this case IPoIB) should allow
>> for the skb already having the maximum number of fragments.
>> Fully linearising the skb is overkill, but I think the first fragment
>> can be added to the linear part of the skb.
>>
>>     David
>>
>>
> When IpoIB handles a skb-request it converts fragments to SGEs to
> be handled by a HCA.
> The problem arises when the HCA have a limited number of SGEs less than
> MAX_SKB_FRAGS.
> (it gets a little worse since IPoIB need to yet another segment)
> I have not found any easy way of fixing this with currenct codebase.

I think because of the complex forwarding nature, a global counter which 
driver's can reduce during initialization time is the only solution I 
see right now without changing the layout of the skb later on.

Unfortunately this doesn't resolve the cases were virtual machines 
inject gso skbs, for those there still needs to be a slow path to do the 
reformatting of the skb. :/

Bye,
Hannes

[toc] | [prev] | [next] | [standalone]

#1308431

On 01/08/2016 12:47 PM, Hannes Frederic Sowa wrote:
> On 08.01.2016 10:55, Hans Westgaard Ry wrote:
>>
>>
>> On 01/06/2016 02:59 PM, David Laight wrote:
>>> From: Hans Westgaard Ry
>>>> Sent: 06 January 2016 13:16
>>>> Devices may have limits on the number of fragments in an skb they
>>>> support. Current codebase uses a constant as maximum for number of
>>>> fragments (MAX_SKB_FRAGS) one skb can hold and use.
>>>>
>>>> When enabling scatter/gather and running traffic with many small
>>>> messages the codebase uses the maximum number of fragments and thereby
>>>> violates the max for certain devices.
>>>>
>>>> An example of such a violation is when running IPoIB on a HCA
>>>> supporting 16 SGE on an architecture with 4K pagesize. The
>>>> MAX_SKB_FRAGS will be 17 (64K/4K+1) and because IPoIB adds yet another
>>>> segment we end up with send_requests with 18 SGE resulting in
>>>> kernel-panic.
>>>>
>>>> The patch allows the device to limit the maximum number fragments used
>>>> in one skb.
>>> This doesn't seem to me to be the correct way to fix this.
>>> Anything that adds an extra fragment (in this case IPoIB) should allow
>>> for the skb already having the maximum number of fragments.
>>> Fully linearising the skb is overkill, but I think the first fragment
>>> can be added to the linear part of the skb.
>>>
>>>     David
>>>
>>>
>> When IpoIB handles a skb-request it converts fragments to SGEs to
>> be handled by a HCA.
>> The problem arises when the HCA have a limited number of SGEs less than
>> MAX_SKB_FRAGS.
>> (it gets a little worse since IPoIB need to yet another segment)
>> I have not found any easy way of fixing this with currenct codebase.
>
> I think because of the complex forwarding nature, a global counter 
> which driver's can reduce during initialization time is the only 
> solution I see right now without changing the layout of the skb later on.
>
> Unfortunately this doesn't resolve the cases were virtual machines 
> inject gso skbs, for those there still needs to be a slow path to do 
> the reformatting of the skb. :/
>
> Bye,
> Hannes
>
>
The use-case for this patch is an application which sends many small 
messages, by write(2) on a TCP socket which has Nagle enabled. A 
scatter-gather capable NIC (potentially also supporting tso) will then 
be asked to send an skb containing up to MAX_SKB_FRAGS worth of 
fragments (17 considering a 4kb page size, hypothetically 65 considering 
an arch supporting 1kb page size).

Now, if the NIC hardware supports less _gather-fragments_, said hardware 
must run with scatter-gather disabled - or - the NIC driver has to 
implement a partial linearization of the skb to reduce #frags to what 
the hardware supports. The latter is far from elegant, and must be 
implemented in all NIC drivers which have this restriction.

This patch provides the flexibility to choose the maximum number of 
fragments that can be passed down to the NIC in order to
utilize the NIC SG hardware features.


In our view we are discussing two different issues:

    1. Is it reasonable that a NIC can restrict #frags in an skb when 
transmitting?
    2. If yes to the above, how is this implemented the best possible way.

Thanks a lot for feedback on the implementation from David Laight, Eric 
Dumazet and Hannes Fredreric Sowa.

What do you think?

        Hans

[toc] | [prev] | [next] | [standalone]

#1308449

Apologies from my prior top post, I used my corporate webmail instead
of my usual email client.

[toc] | [prev] | [next] | [standalone]

#1308451

1) There are no arch with 1K page sizes. Most certainly, if we had
MAX_SKB_FRAGS=65 some assumptions in the stack would fail.

2) TCP stack has coalescing support. write(2) or sendmsg(2) should
append data into the last skb in write queue, and still use 32 KB
frags.
    You get pathological skb when using sendpage() or when one thread
writes data into _multiple_ TCP sockets, since TCP stack uses
    a per thread 32 KB reserve (
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=5640f7685831e088fe6c2e1f863a6805962f8e81
)

2) As I said, implementing a limit in TCP stack is not enough. Your
patch is therefore adding complexity for all users, but is not a
general solution.

   GRO, tun device, many things can still cook 'big skbs'

    You need to properly implement a fallback, possibly using
ndo_features_check(), or directly from your ndo_start_xmit()

3) We currently have a very dumb way to fallback, forcing a linearize
call, likely to fail if memory is fragmented and skb big.

    You could instead provide a smart helper, trying to reduce the
number of frags in a skb by chosing adjacent frags and
re-allocating/merging them.

    By choosing, I mean trying to pick smallest ones to minimize copy
cost, to get one skb with X less fragment. (X=1 in your case ?)

   I know for example that bnx2x could benefit from such a helper, as
it has a 13 frags limits.
   (bnx2x_pkt_req_lin(), called from bnx2x ndo_start_xmit()


On Wed, Jan 13, 2016 at 5:57 AM, Hans Westgaard Ry
<hans.westgaard.ry@oracle.com> wrote:
>
>
> On 01/08/2016 12:47 PM, Hannes Frederic Sowa wrote:
>>
>> On 08.01.2016 10:55, Hans Westgaard Ry wrote:
>>>
>>>
>>>
>>> On 01/06/2016 02:59 PM, David Laight wrote:
>>>>
>>>> From: Hans Westgaard Ry
>>>>>
>>>>> Sent: 06 January 2016 13:16
>>>>> Devices may have limits on the number of fragments in an skb they
>>>>> support. Current codebase uses a constant as maximum for number of
>>>>> fragments (MAX_SKB_FRAGS) one skb can hold and use.
>>>>>
>>>>> When enabling scatter/gather and running traffic with many small
>>>>> messages the codebase uses the maximum number of fragments and thereby
>>>>> violates the max for certain devices.
>>>>>
>>>>> An example of such a violation is when running IPoIB on a HCA
>>>>> supporting 16 SGE on an architecture with 4K pagesize. The
>>>>> MAX_SKB_FRAGS will be 17 (64K/4K+1) and because IPoIB adds yet another
>>>>> segment we end up with send_requests with 18 SGE resulting in
>>>>> kernel-panic.
>>>>>
>>>>> The patch allows the device to limit the maximum number fragments used
>>>>> in one skb.
>>>>
>>>> This doesn't seem to me to be the correct way to fix this.
>>>> Anything that adds an extra fragment (in this case IPoIB) should allow
>>>> for the skb already having the maximum number of fragments.
>>>> Fully linearising the skb is overkill, but I think the first fragment
>>>> can be added to the linear part of the skb.
>>>>
>>>>     David
>>>>
>>>>
>>> When IpoIB handles a skb-request it converts fragments to SGEs to
>>> be handled by a HCA.
>>> The problem arises when the HCA have a limited number of SGEs less than
>>> MAX_SKB_FRAGS.
>>> (it gets a little worse since IPoIB need to yet another segment)
>>> I have not found any easy way of fixing this with currenct codebase.
>>
>>
>> I think because of the complex forwarding nature, a global counter which
>> driver's can reduce during initialization time is the only solution I see
>> right now without changing the layout of the skb later on.
>>
>> Unfortunately this doesn't resolve the cases were virtual machines inject
>> gso skbs, for those there still needs to be a slow path to do the
>> reformatting of the skb. :/
>>
>> Bye,
>> Hannes
>>
>>
> The use-case for this patch is an application which sends many small
> messages, by write(2) on a TCP socket which has Nagle enabled. A
> scatter-gather capable NIC (potentially also supporting tso) will then be
> asked to send an skb containing up to MAX_SKB_FRAGS worth of fragments (17
> considering a 4kb page size, hypothetically 65 considering an arch
> supporting 1kb page size).
>
> Now, if the NIC hardware supports less _gather-fragments_, said hardware
> must run with scatter-gather disabled - or - the NIC driver has to implement
> a partial linearization of the skb to reduce #frags to what the hardware
> supports. The latter is far from elegant, and must be implemented in all NIC
> drivers which have this restriction.
>
> This patch provides the flexibility to choose the maximum number of
> fragments that can be passed down to the NIC in order to
> utilize the NIC SG hardware features.
>
>
> In our view we are discussing two different issues:
>
>    1. Is it reasonable that a NIC can restrict #frags in an skb when
> transmitting?
>    2. If yes to the above, how is this implemented the best possible way.
>
> Thanks a lot for feedback on the implementation from David Laight, Eric
> Dumazet and Hannes Fredreric Sowa.
>
> What do you think?
>
>        Hans
>

[toc] | [prev] | [next] | [standalone]

#1308492

On 13.01.2016 15:19, Eric Dumazet wrote:
> 1) There are no arch with 1K page sizes. Most certainly, if we had
> MAX_SKB_FRAGS=65 some assumptions in the stack would fail.
>
> 2) TCP stack has coalescing support. write(2) or sendmsg(2) should
> append data into the last skb in write queue, and still use 32 KB
> frags.
>      You get pathological skb when using sendpage() or when one thread
> writes data into _multiple_ TCP sockets, since TCP stack uses
>      a per thread 32 KB reserve (
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=5640f7685831e088fe6c2e1f863a6805962f8e81
> )
>
> 2) As I said, implementing a limit in TCP stack is not enough. Your
> patch is therefore adding complexity for all users, but is not a
> general solution.
>
>     GRO, tun device, many things can still cook 'big skbs'
>
>      You need to properly implement a fallback, possibly using
> ndo_features_check(), or directly from your ndo_start_xmit()
>
> 3) We currently have a very dumb way to fallback, forcing a linearize
> call, likely to fail if memory is fragmented and skb big.
>
>      You could instead provide a smart helper, trying to reduce the
> number of frags in a skb by chosing adjacent frags and
> re-allocating/merging them.
>
>      By choosing, I mean trying to pick smallest ones to minimize copy
> cost, to get one skb with X less fragment. (X=1 in your case ?)
>
>     I know for example that bnx2x could benefit from such a helper, as
> it has a 13 frags limits.
>     (bnx2x_pkt_req_lin(), called from bnx2x ndo_start_xmit()

As I proposed, we could globally (or per netns) limit the maximum , I 
think this would be okay and could be the best alternative to install 
slow-paths which could be hit quite constantly.

Otherwise, the fallbacks like Eric proposed them are needed. I do not 
see any other choice.

Thanks,
Hannes

[toc] | [prev] | [next] | [standalone]

#1308511

From: Eric Dumazet <edumazet@google.com>
Date: Wed, 13 Jan 2016 06:19:11 -0800

> 2) TCP stack has coalescing support. write(2) or sendmsg(2) should
> append data into the last skb in write queue, and still use 32 KB
> frags.

Another way to get pathological SKBs is to do lots of tiny sendpage()
calls over discontiguous areas of the file.

[toc] | [prev] | [next] | [standalone]

#1308524

On Wed, Jan 13, 2016 at 7:38 AM, David Miller <davem@davemloft.net> wrote:
> From: Eric Dumazet <edumazet@google.com>
> Date: Wed, 13 Jan 2016 06:19:11 -0800
>
>> 2) TCP stack has coalescing support. write(2) or sendmsg(2) should
>> append data into the last skb in write queue, and still use 32 KB
>> frags.
>
> Another way to get pathological SKBs is to do lots of tiny sendpage()
> calls over discontiguous areas of the file.

Yes, this was what I mentioned in the following sentence.
"You get pathological skb when using sendpage() or ..."

[toc] | [prev] | [next] | [standalone]

#1308818

Hans Westgaard Ry <hans.westgaard.ry@oracle.com> writes:

> On 01/08/2016 12:47 PM, Hannes Frederic Sowa wrote:
>> On 08.01.2016 10:55, Hans Westgaard Ry wrote:
>>>
>>>
>>> On 01/06/2016 02:59 PM, David Laight wrote:
>>>> From: Hans Westgaard Ry
>>>>> Sent: 06 January 2016 13:16
>>>>> Devices may have limits on the number of fragments in an skb they
>>>>> support. Current codebase uses a constant as maximum for number of
>>>>> fragments (MAX_SKB_FRAGS) one skb can hold and use.
>>>>>
>>>>> When enabling scatter/gather and running traffic with many small
>>>>> messages the codebase uses the maximum number of fragments and thereby
>>>>> violates the max for certain devices.
>>>>>
>>>>> An example of such a violation is when running IPoIB on a HCA
>>>>> supporting 16 SGE on an architecture with 4K pagesize. The
>>>>> MAX_SKB_FRAGS will be 17 (64K/4K+1) and because IPoIB adds yet another
>>>>> segment we end up with send_requests with 18 SGE resulting in
>>>>> kernel-panic.
>>>>>
>>>>> The patch allows the device to limit the maximum number fragments used
>>>>> in one skb.
>>>> This doesn't seem to me to be the correct way to fix this.
>>>> Anything that adds an extra fragment (in this case IPoIB) should allow
>>>> for the skb already having the maximum number of fragments.
>>>> Fully linearising the skb is overkill, but I think the first fragment
>>>> can be added to the linear part of the skb.
>>>>
>>>>     David
>>>>
>>>>
>>> When IpoIB handles a skb-request it converts fragments to SGEs to
>>> be handled by a HCA.
>>> The problem arises when the HCA have a limited number of SGEs less than
>>> MAX_SKB_FRAGS.
>>> (it gets a little worse since IPoIB need to yet another segment)
>>> I have not found any easy way of fixing this with currenct codebase.
>>
>> I think because of the complex forwarding nature, a global counter which
>> driver's can reduce during initialization time is the only solution I see
>> right now without changing the layout of the skb later on.
>>
>> Unfortunately this doesn't resolve the cases were virtual machines inject gso
>> skbs, for those there still needs to be a slow path to do the reformatting of
>> the skb. :/
>>
>> Bye,
>> Hannes
>>
>>
> The use-case for this patch is an application which sends many small messages,
> by write(2) on a TCP socket which has Nagle enabled. A scatter-gather capable
> NIC (potentially also supporting tso) will then be asked to send an skb
> containing up to MAX_SKB_FRAGS worth of fragments (17 considering a 4kb page
> size, hypothetically 65 considering an arch supporting 1kb page size).
>
> Now, if the NIC hardware supports less _gather-fragments_, said hardware must
> run with scatter-gather disabled - or - the NIC driver has to implement a
> partial linearization of the skb to reduce #frags to what the hardware
> supports. The latter is far from elegant, and must be implemented in all NIC
> drivers which have this restriction.
>
> This patch provides the flexibility to choose the maximum number of fragments
> that can be passed down to the NIC in order to
> utilize the NIC SG hardware features.
>
>
> In our view we are discussing two different issues:
>
>    1. Is it reasonable that a NIC can restrict #frags in an skb when
> transmitting?
>    2. If yes to the above, how is this implemented the best possible way.
>
> Thanks a lot for feedback on the implementation from David Laight, Eric Dumazet
> and Hannes Fredreric Sowa.
>
> What do you think?

*Scratches my head*  Why doesn't someone fix the infiniband firmware so
that it supports more scatter gather entries?

Last I looked everything like this in infiniband was all implemented in
firmware and there is only one vendor to pick on, so it should be
comparatively easy to just fix the hardware so it does not have this
limitation.

Eric

[toc] | [prev] | [next] | [standalone]

#1302795

On Wed, 2016-01-06 at 14:16 +0100, Hans Westgaard Ry wrote:
> Devices may have limits on the number of fragments in an skb they
> support. Current codebase uses a constant as maximum for number of
> fragments (MAX_SKB_FRAGS) one skb can hold and use.
> 
> When enabling scatter/gather and running traffic with many small
> messages the codebase uses the maximum number of fragments and thereby
> violates the max for certain devices.
> 
> An example of such a violation is when running IPoIB on a HCA
> supporting 16 SGE on an architecture with 4K pagesize. The
> MAX_SKB_FRAGS will be 17 (64K/4K+1) and because IPoIB adds yet another
> segment we end up with send_requests with 18 SGE resulting in
> kernel-panic.
> 
> The patch allows the device to limit the maximum number fragments used
> in one skb.
> 
> The functionality corresponds to gso_max_size/gso_max_segs for gso.

Unfortunately this is not the right place to fix this issue.

Think about forwarding workloads, where the SKB is cooked by GRO engine.

Anyway, local TCP stack uses 32KB page fragments, so typical skb has no
more than 3 frags.

Look at ndo_features_check(), where the problematic device driver can
add its logic.



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1304327

On 01/06/2016 03:05 PM, Eric Dumazet wrote:
> On Wed, 2016-01-06 at 14:16 +0100, Hans Westgaard Ry wrote:
>> Devices may have limits on the number of fragments in an skb they
>> support. Current codebase uses a constant as maximum for number of
>> fragments (MAX_SKB_FRAGS) one skb can hold and use.
>>
>> When enabling scatter/gather and running traffic with many small
>> messages the codebase uses the maximum number of fragments and thereby
>> violates the max for certain devices.
>>
>> An example of such a violation is when running IPoIB on a HCA
>> supporting 16 SGE on an architecture with 4K pagesize. The
>> MAX_SKB_FRAGS will be 17 (64K/4K+1) and because IPoIB adds yet another
>> segment we end up with send_requests with 18 SGE resulting in
>> kernel-panic.
>>
>> The patch allows the device to limit the maximum number fragments used
>> in one skb.
>>
>> The functionality corresponds to gso_max_size/gso_max_segs for gso.
> Unfortunately this is not the right place to fix this issue.
>
> Think about forwarding workloads, where the SKB is cooked by GRO engine.
>
> Anyway, local TCP stack uses 32KB page fragments, so typical skb has no
> more than 3 frags.
>
> Look at ndo_features_check(), where the problematic device driver can
> add its logic.
>
>
>
I've had a look at ndo_features_check and understand that I could supply 
my own
version of the routine, but I wasn't able to figure out how that would 
solve my problem.
As far as I can see the routine is not called in the part of code 
handling scatter/gather.
Could you help out with more info?


Hans

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web