Praveen Kaligineedi wrote:
From: Bailey Forrest bcf@google.com
The NIC requires each TSO segment to not span more than 10 descriptors. gve_can_send_tso() performs this check. However, the current code misses an edge case when a TSO skb has a large frag that needs to be split into multiple descriptors
because each descriptor may not exceed 16KB (GVE_TX_MAX_BUF_SIZE_DQO)
, causing the 10 descriptor limit per TSO-segment to be exceeded. This change fixes the edge case.
Fixes: a57e5de476be ("gve: DQO: Add TX path") Signed-off-by: Praveen Kaligineedi pkaligineedi@google.com Signed-off-by: Bailey Forrest bcf@google.com Reviewed-by: Jeroen de Borst jeroendb@google.com
drivers/net/ethernet/google/gve/gve_tx_dqo.c | 22 +++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/google/gve/gve_tx_dqo.c b/drivers/net/ethernet/google/gve/gve_tx_dqo.c index 0b3cca3fc792..dc39dc481f21 100644 --- a/drivers/net/ethernet/google/gve/gve_tx_dqo.c +++ b/drivers/net/ethernet/google/gve/gve_tx_dqo.c @@ -866,22 +866,42 @@ static bool gve_can_send_tso(const struct sk_buff *skb) const int header_len = skb_tcp_all_headers(skb); const int gso_size = shinfo->gso_size; int cur_seg_num_bufs;
- int last_frag_size;
nit: last_frag can be interpreted as frags[nr_frags - 1], perhaps prev_frag.
int cur_seg_size; int i; cur_seg_size = skb_headlen(skb) - header_len;
- last_frag_size = skb_headlen(skb); cur_seg_num_bufs = cur_seg_size > 0;
for (i = 0; i < shinfo->nr_frags; i++) { if (cur_seg_size >= gso_size) { cur_seg_size %= gso_size; cur_seg_num_bufs = cur_seg_size > 0;
/* If the last buffer is split in the middle of a TSO
s/buffer/frag?
* segment, then it will count as two descriptors.*/if (last_frag_size > GVE_TX_MAX_BUF_SIZE_DQO) {int last_frag_remain = last_frag_size %GVE_TX_MAX_BUF_SIZE_DQO;/* If the last frag was evenly divisible by* GVE_TX_MAX_BUF_SIZE_DQO, then it will not be* split in the current segment.
Is this true even if the segment did not start at the start of the frag?
Overall, it's not trivial to follow. Probably because the goal is to count max descriptors per segment, but that is not what is being looped over.
Intuitive (perhaps buggy, a quick sketch), this is what is intended, right?
static bool gve_can_send_tso(const struct sk_buff *skb) { int frag_size = skb_headlen(skb) - header_len; int gso_size_left; int frag_idx = 0; int num_desc; int desc_len; int off = 0;
while (off < skb->len) { gso_size_left = shinfo->gso_size; num_desc = 0;
while (gso_size_left) { desc_len = min(gso_size_left, frag_size); gso_size_left -= desc_len; frag_size -= desc_len; num_desc++;
if (num_desc > max_descs_per_seg) return false;
if (!frag_size) frag_size = skb_frag_size(&shinfo->frags[frag_idx++]); } }
return true; }
This however loops skb->len / gso_size. While the above modulo operation skips many segments that span a frag. Not sure if the more intuitive approach could be as performant.
Else, I'll stare some more at the suggested patch to convince myself that it is correct and complete..
*/
*/if (last_frag_remain &&cur_seg_size > last_frag_remain) {cur_seg_num_bufs++;}}if (unlikely(++cur_seg_num_bufs > max_bufs_per_seg)) return false;
cur_seg_size += skb_frag_size(&shinfo->frags[i]);
last_frag_size = skb_frag_size(&shinfo->frags[i]);cur_seg_size += last_frag_size;return true; -- 2.45.2.993.g49e7a77208-goog