On Mon, 04 Nov 2019 10:56:52 -0800, John Fastabend wrote:
> Jakub Kicinski wrote:
> > 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?
