On Fri, Jul 19, 2024 at 9:56 AM Bailey Forrest bcf@google.com wrote:
On Fri, Jul 19, 2024 at 7:31 AM Praveen Kaligineedi pkaligineedi@google.com wrote:
On Thu, Jul 18, 2024 at 8:47 PM Willem de Bruijn willemdebruijn.kernel@gmail.com wrote:
On Thu, Jul 18, 2024 at 9:52 PM Praveen Kaligineedi pkaligineedi@google.com wrote:
On Thu, Jul 18, 2024 at 4:07 PM Willem de Bruijn willemdebruijn.kernel@gmail.com wrote:
* 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?
The comment probably is a bit confusing here. The current segment we are tracking could have a portion in the previous frag. The code assumed that the portion on the previous frag (if present) mapped to only one descriptor. However, that portion could have been split across two descriptors due to the restriction that each descriptor cannot exceed 16KB.
/* If the last frag was evenly divisible by
* GVE_TX_MAX_BUF_SIZE_DQO, then it will not be* split in the current segment.This is true because the smallest multiple of 16KB is 32KB, and the largest gso_size at least for Ethernet will be 9K. But I don't think that that is what is used here as the basis for this statement?
The largest Ethernet gso_size (9K) is less than GVE_TX_MAX_BUF_SIZE_DQO is an implicit assumption made in this patch and in that comment. Bailey, please correct me if I am wrong..
If last_frag_size is evenly divisible by GVE_TX_MAX_BUF_SIZE_DQO, it doesn't hit the edge case we're looking for.
- If it's evenly divisible, then we know it will use exactly
(last_frag_size / GVE_TX_MAX_BUF_SIZE_DQO) descriptors
This assumes that gso_segment start is aligned with skb_frag start. That is not necessarily true, right?
If headlen minus protocol headers is 1B, then the first segment will have two descriptors { 1B, 9KB - 1 }. And the next segment can have skb_frag_size - ( 9KB - 1).
I think the statement is correct, but because every multiple of 16KB is so much larger than the max gso_size of ~9KB, that a single segment will never include more than two skb_frags.
Quite possibly the code overestimates the number of descriptors per segment now, but that is safe and only a performance regression.
- GVE_TX_MAX_BUF_SIZE_DQO > 9k, so we know each descriptor won't
create a segment which exceeds the limit
For a net patch, it is generally better to make a small fix rather than rewrite.
That said, my sketch without looping over every segment:
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_left); gso_size_left -= desc_len; frag_size_left -= desc_len; num_desc++;
if (num_desc > max_descs_per_seg) return false;
if (!frag_size_left) frag_size_left = skb_frag_size(&shinfo->frags[frag_idx++]); + else + frag_size_left %= gso_size; /* skip segments that fit in one desc */ } }