On Mon, Nov 26, 2018 at 10:46:33PM +0900, Xin Long wrote:
> On Mon, Nov 26, 2018 at 9:54 PM Neil Horman <nhorman@xxxxxxxxxxxxx> wrote:
> >
> > On Mon, Nov 26, 2018 at 07:22:05PM +0800, Xin Long wrote:
> > > Now when using stream reconfig to add out streams, stream->out
> > > will get re-allocated, and all old streams' information will
> > > be copied to the new ones and the old ones will be freed.
> > >
> > > So without stream->out_curr updated, next time when trying to
> > > send from stream->out_curr stream, a panic would be caused.
> > >
> > > This patch is to define sctp_stream_out_copy used to update the
> > > stream->out_curr pointer to the new stream when copying the old
> > > streams' information.
> > >
> > > While at it, rename fa_copy to sctp_stream_in_copy.
> > >
> > > Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
> > > Reported-by: Ying Xu <yinxu@xxxxxxxxxx>
> > > Reported-by: syzbot+e33a3a138267ca119c7d@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > Signed-off-by: Xin Long <lucien.xin@xxxxxxxxx>
> > > ---
> > > net/sctp/stream.c | 46 ++++++++++++++++++++++++++++++++--------------
> > > 1 file changed, 32 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> > > index 3892e76..0687eeb 100644
> > > --- a/net/sctp/stream.c
> > > +++ b/net/sctp/stream.c
> > > @@ -61,18 +61,6 @@ static void fa_free(struct flex_array *fa)
> > > flex_array_free(fa);
> > > }
> > >
> > > -static void fa_copy(struct flex_array *fa, struct flex_array *from,
> > > - size_t index, size_t count)
> > > -{
> > > - void *elem;
> > > -
> > > - while (count--) {
> > > - elem = flex_array_get(from, index);
> > > - flex_array_put(fa, index, elem, 0);
> > > - index++;
> > > - }
> > > -}
> > > -
> > > static void fa_zero(struct flex_array *fa, size_t index, size_t count)
> > > {
> > > void *elem;
> > > @@ -135,6 +123,36 @@ static void sctp_stream_outq_migrate(struct sctp_stream *stream,
> > > kfree(SCTP_SO(stream, i)->ext);
> > > }
> > >
> > > +static void sctp_stream_in_copy(struct flex_array *fa,
> > > + struct sctp_stream *stream, __u16 count)
> > > +{
> > > + size_t index = 0;
> > > + void *elem;
> > > +
> > > + count = min(count, stream->incnt);
> > > + while (count--) {
> > > + elem = flex_array_get(stream->in, index);
> > > + flex_array_put(fa, index, elem, 0);
> > > + index++;
> > > + }
> > > +}
> > > +
> > > +static void sctp_stream_out_copy(struct flex_array *fa,
> > > + struct sctp_stream *stream, __u16 count)
> > > +{
> > > + size_t index = 0;
> > > + void *elem;
> > > +
> > > + count = min(count, stream->outcnt);
> > > + while (count--) {
> > > + elem = flex_array_get(stream->out, index);
> > > + flex_array_put(fa, index, elem, 0);
> > > + if (stream->out_curr == elem)
> > > + stream->out_curr = flex_array_get(fa, index);
> > > + index++;
> > > + }
> > > +}
> > > +
> > Seems like you are duplicating code here. I think you would be better off
> > moving the fa_copy routine to the flex_array api (perhaps renaming it
> > flex_array_copy), and then codig sctp_stream_*_copy as static inlines that just
> > call the flex_array api to do the copy. As for setting the out_curr pointer,
> > perhaps you should convert that to an index, so it can be looked up on demand,
> changing to use index only for this may not worth it.
> there is no API from flex_array to convert element to index either
> the index is also the stream_id, but we didn't save it into stream_out
> either, too.
>
Right, I'm saying it would be valuable to augment the flex_array api to include
a copy function, as well as a pointer to index lookup element, that could have
use beyond just sctp.
> > so that it doesn't have to be updated here at all, or alternatively, just set it
> > back to NULL here so that the selected scheduler will be forced to do the next
> > lookup.
> We can't set it back to NULL. Otherwise, the scheduler may go to
> send other msg if the last msg (with multiple chunks) is not yet sent
> out completely, which is not allowed when it's not I-Data chunk.
>
If setting it back to NULL isn't valid, then the above is your solution.
> This is not much duplicating, and this can reduce few params.
> I'm actually ok with this.
>
I know your ok with it, you wrote the patch :). I however, am not really ok
with the duplication. I would like to see it collapsed, if its not going to
create a significant performance impact.
Neil
