Praveen Kaligineedi wrote:
> From: Bailey Forrest <bcf@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: Bailey Forrest <bcf@xxxxxxxxxx>
> Reviewed-by: Jeroen de Borst <jeroendb@xxxxxxxxxx>
> ---
> 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
>
