On Mon, 04 Nov 2019 10:56:52 -0800, John Fastabend wrote:

Jakub Kicinski wrote:

quoted

diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index cf390e0aa73d..f87fde3a846c 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c

@@ -270,18 +270,28 @@ void sk_msg_trim(struct sock *sk, struct sk_msg *msg, int len)
 
 	msg->sg.data[i].length -= trim;
 	sk_mem_uncharge(sk, trim);
+	/* Adjust copybreak if it falls into the trimmed part of last buf */
+	if (msg->sg.curr == i && msg->sg.copybreak > msg->sg.data[i].length)
+		msg->sg.copybreak = msg->sg.data[i].length;
 out:
-	/* If we trim data before curr pointer update copybreak and current
-	 * so that any future copy operations start at new copy location.
+	sk_msg_iter_var_next(i);
+	msg->sg.end = i;
+
+	/* If we trim data a full sg elem before curr pointer update
+	 * copybreak and current so that any future copy operations
+	 * start at new copy location.
 	 * However trimed data that has not yet been used in a copy op
 	 * does not require an update.
 	 */
-	if (msg->sg.curr >= i) {
+	if (!msg->sg.size) {
+		msg->sg.curr = msg->sg.start;
+		msg->sg.copybreak = 0;
+	} else if (sk_msg_iter_dist(msg->sg.start, msg->sg.curr) >
+		   sk_msg_iter_dist(msg->sg.end, msg->sg.curr)) {  

I'm not seeing how this can work. Taking simple case with start < end
so normal geometry without wrapping. Let,

 start = 1
 curr  = 3
 end   = 4

We could trim an index to get,

 start = 1
  curr = 3
     i = 3
   end = 4

IOW like this?

	test_one(/* start */ 1, /* curr */ 3, /* copybreak */ 150,
		 /* trim */ 500,
		 /* curr */ 3, /* copybreak */ 100, /* end */ 4,
		 /* data */ 200, 200, 200);

test #13  start:1 curr:3 end:4 cb:150 size: 600      0 200 200 200   0	OKAY

Then after out: label this would push end up one,

 start = 1
  curr = 3
     i = 3
   end = 4

I moved the assignment to end before the curr adjustment, so 'i' is
equivalent to 'end' at this point.

But dist(start,curr) = 2 and dist(end, curr) = 1 and we would set curr
to '3' but clear the copybreak?

I don't think we'd fall into this condition ever, unless we moved end.
And in your example AFAIU we don't move end.

I think a better comparison would be,

  if (sk_msg_iter_dist(msg->sg.start, i) <
      sk_msg_iter_dist(msg->sg.start, msg->sg.curr)

To check if 'i' walked past curr so we can reset curr/copybreak?

Ack, this does read better!

Should we use <= here? If we dropped a full segment, should curr point
at the end of the last remaining segment or should it point at 0 in end?