[PATCHv2,net-next,04/12] sctp: implement make_datafrag for sctp_stream_interleave

To avoid hundreds of checks for the different process on I-DATA chunk,
struct sctp_stream_interleave is defined as a group of functions used
to replace the codes in some place where it needs to do different job
according to if the asoc intl_enabled is set.

With these ops, it only needs to initialize asoc->stream.si with
sctp_stream_interleave_0 for normal data if asoc intl_enable is 0,
or sctp_stream_interleave_1 for idata if asoc intl_enable is set in
sctp_stream_init.

After that, the members in asoc->stream.si can be used directly in
some special places without checking asoc intl_enable.

make_datafrag is the first member for sctp_stream_interleave, it's
used to make data or idata frags, called in sctp_datamsg_from_user.
The old function sctp_make_datafrag_empty needs to be adjust some
to fit in this ops.

Note that as idata and data chunks have different length, it also
defines data_chunk_len for sctp_stream_interleave to describe the
chunk size.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
---
 include/net/sctp/sm.h                |  5 +--
 include/net/sctp/stream_interleave.h | 44 ++++++++++++++++++++
 include/net/sctp/structs.h           | 12 ++++++
 net/sctp/Makefile                    |  2 +-
 net/sctp/chunk.c                     |  6 +--
 net/sctp/sm_make_chunk.c             | 21 ++++------
 net/sctp/stream.c                    |  1 +
 net/sctp/stream_interleave.c         | 79 ++++++++++++++++++++++++++++++++++++
 8 files changed, 149 insertions(+), 21 deletions(-)
 create mode 100644 include/net/sctp/stream_interleave.h
 create mode 100644 net/sctp/stream_interleave.c

From: Xin Long
> Sent: 08 December 2017 13:04
...
> @@ -264,8 +264,8 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
>  				frag |= SCTP_DATA_SACK_IMM;
>  		}
> 
> -		chunk = sctp_make_datafrag_empty(asoc, sinfo, len, frag,
> -						 0, GFP_KERNEL);
> +		chunk = asoc->stream.si->make_datafrag(asoc, sinfo, len, frag,
> +						       GFP_KERNEL);

I know that none of the sctp code is very optimised, but that indirect
call is going to be horrid.

	David

On Fri, Dec 08, 2017 at 02:06:04PM +0000, David Laight wrote:
> From: Xin Long
> > Sent: 08 December 2017 13:04
> ...
> > @@ -264,8 +264,8 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
> >  				frag |= SCTP_DATA_SACK_IMM;
> >  		}
> > 
> > -		chunk = sctp_make_datafrag_empty(asoc, sinfo, len, frag,
> > -						 0, GFP_KERNEL);
> > +		chunk = asoc->stream.si->make_datafrag(asoc, sinfo, len, frag,
> > +						       GFP_KERNEL);
> 
> I know that none of the sctp code is very optimised, but that indirect
> call is going to be horrid.

Yeah.. but there is no way to avoid the double derreference
considering we only have the asoc pointer in there and we have to
reach the contents of the data chunk operations struct, and the .si
part is the same as 'stream' part as it's a constant offset.

Due to the for() in there, we could add a variable to store
asoc->stream.si outside the for and then we can do only a single deref
inside it. Xin, can you please try and see if the generated code is
different?

Other suggestions?

  Marcelo

From: Marcelo Ricardo Leitner
> Sent: 08 December 2017 14:57
> 
> On Fri, Dec 08, 2017 at 02:06:04PM +0000, David Laight wrote:
> > From: Xin Long
> > > Sent: 08 December 2017 13:04
> > ...
> > > @@ -264,8 +264,8 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
> > >  				frag |= SCTP_DATA_SACK_IMM;
> > >  		}
> > >
> > > -		chunk = sctp_make_datafrag_empty(asoc, sinfo, len, frag,
> > > -						 0, GFP_KERNEL);
> > > +		chunk = asoc->stream.si->make_datafrag(asoc, sinfo, len, frag,
> > > +						       GFP_KERNEL);
> >
> > I know that none of the sctp code is very optimised, but that indirect
> > call is going to be horrid.
> 
> Yeah.. but there is no way to avoid the double derreference
> considering we only have the asoc pointer in there and we have to
> reach the contents of the data chunk operations struct, and the .si
> part is the same as 'stream' part as it's a constant offset.
...

It isn't only the double indirect, the indirect call itself isn't 'fun'.

I think there are other hot paths where you've replaced a sizeof()
with a ?: clause.
Caching the result might be much better.

	David

On Fri, Dec 08, 2017 at 03:01:31PM +0000, David Laight wrote:
> From: Marcelo Ricardo Leitner
> > Sent: 08 December 2017 14:57
> > 
> > On Fri, Dec 08, 2017 at 02:06:04PM +0000, David Laight wrote:
> > > From: Xin Long
> > > > Sent: 08 December 2017 13:04
> > > ...
> > > > @@ -264,8 +264,8 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
> > > >  				frag |= SCTP_DATA_SACK_IMM;
> > > >  		}
> > > >
> > > > -		chunk = sctp_make_datafrag_empty(asoc, sinfo, len, frag,
> > > > -						 0, GFP_KERNEL);
> > > > +		chunk = asoc->stream.si->make_datafrag(asoc, sinfo, len, frag,
> > > > +						       GFP_KERNEL);
> > >
> > > I know that none of the sctp code is very optimised, but that indirect
> > > call is going to be horrid.
> > 
> > Yeah.. but there is no way to avoid the double derreference
> > considering we only have the asoc pointer in there and we have to
> > reach the contents of the data chunk operations struct, and the .si
> > part is the same as 'stream' part as it's a constant offset.
> ...
> 
> It isn't only the double indirect, the indirect call itself isn't 'fun'.

I meant in this context.

The indirect call is so we don't have to flood the stack with
if (old data chunk fmt) {
	...
} else {
	...
}

So instead of this, we now have some key operations identified and
wrapped up behind this struct, allowing us to abstract whatever data
chunk format it is.

> 
> I think there are other hot paths where you've replaced a sizeof()
> with a ?: clause.
> Caching the result might be much better.

The only new ?: clause I could find this patchset is on patch 12 and
has nothing to do with sizeof().

The sizeof() results are indeed cached, as you can see in patch 4:
+static struct sctp_stream_interleave sctp_stream_interleave_0 = {
+       .data_chunk_len         = sizeof(struct sctp_data_chunk),
and the two helpers on it at the begining of the patch.

  Marcelo

From: 'Marcelo Ricardo Leitner'
> Sent: 08 December 2017 15:16
> On Fri, Dec 08, 2017 at 03:01:31PM +0000, David Laight wrote:
> > From: Marcelo Ricardo Leitner
> > > Sent: 08 December 2017 14:57
> > >
> > > On Fri, Dec 08, 2017 at 02:06:04PM +0000, David Laight wrote:
> > > > From: Xin Long
> > > > > Sent: 08 December 2017 13:04
> > > > ...
> > > > > @@ -264,8 +264,8 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
> > > > >  				frag |= SCTP_DATA_SACK_IMM;
> > > > >  		}
> > > > >
> > > > > -		chunk = sctp_make_datafrag_empty(asoc, sinfo, len, frag,
> > > > > -						 0, GFP_KERNEL);
> > > > > +		chunk = asoc->stream.si->make_datafrag(asoc, sinfo, len, frag,
> > > > > +						       GFP_KERNEL);
> > > >
> > > > I know that none of the sctp code is very optimised, but that indirect
> > > > call is going to be horrid.
> > >
> > > Yeah.. but there is no way to avoid the double derreference
> > > considering we only have the asoc pointer in there and we have to
> > > reach the contents of the data chunk operations struct, and the .si
> > > part is the same as 'stream' part as it's a constant offset.
> > ...
> >
> > It isn't only the double indirect, the indirect call itself isn't 'fun'.
> 
> I meant in this context.
> 
> The indirect call is so we don't have to flood the stack with
> if (old data chunk fmt) {
> 	...
> } else {
> 	...
> }
> 
> So instead of this, we now have some key operations identified and
> wrapped up behind this struct, allowing us to abstract whatever data
> chunk format it is.

Nothing wrong with:
#define foo(asoc, ...) \
	if (asoc->new_fmt) \
		foo_new(asoc, __VA_LIST__); \
	else \
		foo_old(asoc, __VA_LIST__);

> > I think there are other hot paths where you've replaced a sizeof()
> > with a ?: clause.
> > Caching the result might be much better.
> 
> The only new ?: clause I could find this patchset is on patch 12 and
> has nothing to do with sizeof().
> 
> The sizeof() results are indeed cached, as you can see in patch 4:
> +static struct sctp_stream_interleave sctp_stream_interleave_0 = {
> +       .data_chunk_len         = sizeof(struct sctp_data_chunk),
> and the two helpers on it at the begining of the patch.

I was getting two bits mixed up.
But the code that reads data_chunk_len is following an awful lot of pointers.

	David

On Fri, Dec 08, 2017 at 12:56:30PM -0200, Marcelo Ricardo Leitner wrote:
> On Fri, Dec 08, 2017 at 02:06:04PM +0000, David Laight wrote:
> > From: Xin Long
> > > Sent: 08 December 2017 13:04
> > ...
> > > @@ -264,8 +264,8 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
> > >  				frag |= SCTP_DATA_SACK_IMM;
> > >  		}
> > > 
> > > -		chunk = sctp_make_datafrag_empty(asoc, sinfo, len, frag,
> > > -						 0, GFP_KERNEL);
> > > +		chunk = asoc->stream.si->make_datafrag(asoc, sinfo, len, frag,
> > > +						       GFP_KERNEL);
> > 
> > I know that none of the sctp code is very optimised, but that indirect
> > call is going to be horrid.
> 
> Yeah.. but there is no way to avoid the double derreference
> considering we only have the asoc pointer in there and we have to
> reach the contents of the data chunk operations struct, and the .si
> part is the same as 'stream' part as it's a constant offset.
> 
> Due to the for() in there, we could add a variable to store
> asoc->stream.si outside the for and then we can do only a single deref
> inside it. Xin, can you please try and see if the generated code is
> different?
> 
> Other suggestions?
> 
Is it worth replacing the si struct with an index/enum value, and indexing an
array of method pointer structs?  That would save you at least one dereference.

Alternatively you could preform the dereference in two steps (i.e. declare an si
pointer on the stack and set it equal to asoc->stream.si, then deref
si->make_datafrag at call time.  That will at least give the compiler an
opportunity to preload the first pointer.

Neil

>   Marcelo
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

On Fri, Dec 08, 2017 at 10:37:34AM -0500, Neil Horman wrote:
> On Fri, Dec 08, 2017 at 12:56:30PM -0200, Marcelo Ricardo Leitner wrote:
> > On Fri, Dec 08, 2017 at 02:06:04PM +0000, David Laight wrote:
> > > From: Xin Long
> > > > Sent: 08 December 2017 13:04
> > > ...
> > > > @@ -264,8 +264,8 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
> > > >  				frag |= SCTP_DATA_SACK_IMM;
> > > >  		}
> > > > 
> > > > -		chunk = sctp_make_datafrag_empty(asoc, sinfo, len, frag,
> > > > -						 0, GFP_KERNEL);
> > > > +		chunk = asoc->stream.si->make_datafrag(asoc, sinfo, len, frag,
> > > > +						       GFP_KERNEL);
> > > 
> > > I know that none of the sctp code is very optimised, but that indirect
> > > call is going to be horrid.
> > 
> > Yeah.. but there is no way to avoid the double derreference
> > considering we only have the asoc pointer in there and we have to
> > reach the contents of the data chunk operations struct, and the .si
> > part is the same as 'stream' part as it's a constant offset.
> > 
> > Due to the for() in there, we could add a variable to store
> > asoc->stream.si outside the for and then we can do only a single deref
> > inside it. Xin, can you please try and see if the generated code is
> > different?
> > 
> > Other suggestions?
> > 
> Is it worth replacing the si struct with an index/enum value, and indexing an
> array of method pointer structs?  That would save you at least one dereference.

Hmmm, maybe, yes. It would be like
sctp_stream_interleave[asoc->stream.si].make_datafrag(...)

Then same goes for pf->af, probably.

> 
> Alternatively you could preform the dereference in two steps (i.e. declare an si
> pointer on the stack and set it equal to asoc->stream.si, then deref
> si->make_datafrag at call time.  That will at least give the compiler an
> opportunity to preload the first pointer.

Yep, that was my 2nd paragraph above :-) but it only works for cases
such as this one.

  Marcelo

> 
> Neil
> 
> >   Marcelo
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

On Fri, Dec 08, 2017 at 03:32:54PM +0000, David Laight wrote:
> From: 'Marcelo Ricardo Leitner'
> > Sent: 08 December 2017 15:16
> > On Fri, Dec 08, 2017 at 03:01:31PM +0000, David Laight wrote:
> > > From: Marcelo Ricardo Leitner
> > > > Sent: 08 December 2017 14:57
> > > >
> > > > On Fri, Dec 08, 2017 at 02:06:04PM +0000, David Laight wrote:
> > > > > From: Xin Long
> > > > > > Sent: 08 December 2017 13:04
> > > > > ...
> > > > > > @@ -264,8 +264,8 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
> > > > > >  				frag |= SCTP_DATA_SACK_IMM;
> > > > > >  		}
> > > > > >
> > > > > > -		chunk = sctp_make_datafrag_empty(asoc, sinfo, len, frag,
> > > > > > -						 0, GFP_KERNEL);
> > > > > > +		chunk = asoc->stream.si->make_datafrag(asoc, sinfo, len, frag,
> > > > > > +						       GFP_KERNEL);
> > > > >
> > > > > I know that none of the sctp code is very optimised, but that indirect
> > > > > call is going to be horrid.
> > > >
> > > > Yeah.. but there is no way to avoid the double derreference
> > > > considering we only have the asoc pointer in there and we have to
> > > > reach the contents of the data chunk operations struct, and the .si
> > > > part is the same as 'stream' part as it's a constant offset.
> > > ...
> > >
> > > It isn't only the double indirect, the indirect call itself isn't 'fun'.
> > 
> > I meant in this context.
> > 
> > The indirect call is so we don't have to flood the stack with
> > if (old data chunk fmt) {
> > 	...
> > } else {
> > 	...
> > }
> > 
> > So instead of this, we now have some key operations identified and
> > wrapped up behind this struct, allowing us to abstract whatever data
> > chunk format it is.
> 
> Nothing wrong with:
> #define foo(asoc, ...) \
> 	if (asoc->new_fmt) \

Not all function pointers in sctp_stream_interleave have asoc as
a parameter, so we would have to have something like:

#define foo_asoc(asoc, ...) \
	if (asoc->new_fmt) \
		...

#define foo_chunk(chunk, ...) \
	if (chunk->asoc->new_fmt) \
		...

#define foo_ulpq(ulpq, ...) \
	if (ulpq->asoc->new_fmt) \
		...

and we're pretty much back to double deref.
Maybe some reworking on the parameters could alleviate some of these,
but not all.

> 		foo_new(asoc, __VA_LIST__); \
> 	else \
> 		foo_old(asoc, __VA_LIST__);
> 
> > > I think there are other hot paths where you've replaced a sizeof()
> > > with a ?: clause.
> > > Caching the result might be much better.
> > 
> > The only new ?: clause I could find this patchset is on patch 12 and
> > has nothing to do with sizeof().
> > 
> > The sizeof() results are indeed cached, as you can see in patch 4:
> > +static struct sctp_stream_interleave sctp_stream_interleave_0 = {
> > +       .data_chunk_len         = sizeof(struct sctp_data_chunk),
> > and the two helpers on it at the begining of the patch.
> 
> I was getting two bits mixed up.
> But the code that reads data_chunk_len is following an awful lot of pointers.

From path 4:
        max_data = asoc->pathmtu -
                   sctp_sk(asoc->base.sk)->pf->af->net_header_len -
-                  sizeof(struct sctphdr) - sizeof(struct sctp_data_chunk);
+                  sizeof(struct sctphdr) - sctp_datachk_len(&asoc->stream);

You're worried with the double deref in sctp_datachk_len() but on the
line right above it we have 4 derefs. There are several other cases
of double deref in sctp stack even on hot path. Not sure why you're
picking on this one, but ok.

  Marcelo

From: Marcelo Ricardo Leitner
> Sent: 08 December 2017 16:00
...
> > Is it worth replacing the si struct with an index/enum value, and indexing an
> > array of method pointer structs?  That would save you at least one dereference.
> 
> Hmmm, maybe, yes. It would be like
> sctp_stream_interleave[asoc->stream.si].make_datafrag(...)

If you only expect 2 choices then an if () is likely
to produce better code that the above.

The actual implementation can be hidden inside a #define
or static inline function.

	David

On Fri, Dec 08, 2017 at 04:04:58PM +0000, David Laight wrote:
> From: Marcelo Ricardo Leitner
> > Sent: 08 December 2017 16:00
> ...
> > > Is it worth replacing the si struct with an index/enum value, and indexing an
> > > array of method pointer structs?  That would save you at least one dereference.
> > 
> > Hmmm, maybe, yes. It would be like
> > sctp_stream_interleave[asoc->stream.si].make_datafrag(...)
> 
> If you only expect 2 choices then an if () is likely
> to produce better code that the above.
> 
> The actual implementation can be hidden inside a #define
> or static inline function.
> 
Thats the real question though, will we expect more than two interleaving
strategies?  Currently its a boolean operation so the answer seems like yes, but
is there a possiblity of a biased interleaving, or other worthwhile algorithm?

Neil

> 	David
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

On Sat, Dec 9, 2017 at 12:00 AM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> On Fri, Dec 08, 2017 at 10:37:34AM -0500, Neil Horman wrote:
>> On Fri, Dec 08, 2017 at 12:56:30PM -0200, Marcelo Ricardo Leitner wrote:
>> > On Fri, Dec 08, 2017 at 02:06:04PM +0000, David Laight wrote:
>> > > From: Xin Long
>> > > > Sent: 08 December 2017 13:04
>> > > ...
>> > > > @@ -264,8 +264,8 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
>> > > >                                 frag |= SCTP_DATA_SACK_IMM;
>> > > >                 }
>> > > >
>> > > > -               chunk = sctp_make_datafrag_empty(asoc, sinfo, len, frag,
>> > > > -                                                0, GFP_KERNEL);
>> > > > +               chunk = asoc->stream.si->make_datafrag(asoc, sinfo, len, frag,
>> > > > +                                                      GFP_KERNEL);
>> > >
>> > > I know that none of the sctp code is very optimised, but that indirect
>> > > call is going to be horrid.
>> >
>> > Yeah.. but there is no way to avoid the double derreference
>> > considering we only have the asoc pointer in there and we have to
>> > reach the contents of the data chunk operations struct, and the .si
>> > part is the same as 'stream' part as it's a constant offset.
>> >
>> > Due to the for() in there, we could add a variable to store
>> > asoc->stream.si outside the for and then we can do only a single deref
>> > inside it. Xin, can you please try and see if the generated code is
>> > different?
>> >
>> > Other suggestions?
>> >
>> Is it worth replacing the si struct with an index/enum value, and indexing an
>> array of method pointer structs?  That would save you at least one dereference.
>
> Hmmm, maybe, yes. It would be like
> sctp_stream_interleave[asoc->stream.si].make_datafrag(...)
>
> Then same goes for pf->af, probably.
>
>>
>> Alternatively you could preform the dereference in two steps (i.e. declare an si
>> pointer on the stack and set it equal to asoc->stream.si, then deref
>> si->make_datafrag at call time.  That will at least give the compiler an
>> opportunity to preload the first pointer.
>
> Yep, that was my 2nd paragraph above :-) but it only works for cases
> such as this one.

Now:
  for(N) {
    ...
    chunk = asoc->stream.si->make_datafrag(asoc, sinfo, len, frag,
     0x000000000000fb58 <+360>: mov    0x848(%r13),%rax  <---- [a]
     0x000000000000fb5f <+367>: movzbl %cl,%ecx
     0x000000000000fb62 <+370>: mov    $0x14000c0,%r8d
     0x000000000000fb68 <+376>: mov    %r12d,%edx
     0x000000000000fb6b <+379>: mov    (%rsp),%rsi
     0x000000000000fb6f <+383>: mov    %r13,%rdi  <=(X)
     0x000000000000fb72 <+386>: callq  *0x8(%rax)  <---- [b]
     0x000000000000fb78 <+392>: mov    %rax,%r15
   }

   ret = N * ([a] + [b])


After using a variable:
  struct sctp_stream_interleave *si;
  ...
  si = asoc->stream.si;
     0x000000000000fb44 <+340>: mov    0x848(%r14),%rax
     0x000000000000fb4e <+350>: mov    %rax,0x20(%rsp) <----- [1]

  for(N) {
    ...
    chunk = si->make_datafrag(asoc, sinfo, len, frag, GFP_KERNEL);
     0x000000000000fb69 <+377>: mov    0x20(%rsp),%rax <----- [2]
     0x000000000000fb6e <+382>: movzbl %cl,%ecx
     0x000000000000fb71 <+385>: mov    $0x14000c0,%r8d
     0x000000000000fb77 <+391>: mov    %r12d,%edx
     0x000000000000fb7a <+394>: mov    (%rsp),%rsi
     0x000000000000fb7e <+398>: mov    0x28(%rsp),%rdi <=(Y)
     0x000000000000fb83 <+403>: callq  *0x8(%rax) <----- [3]
     0x000000000000fb89 <+409>: mov    %rax,%r14
   }

   ret = [1] + N * ([2] + [3])


Another small difference:
  as you can see, comparing to (X), (Y) is using 0x28(%rsp) in the loop,
  instead of %r13.

So that's what I can see from the related generated code.
If 0x848(%r13) is not worse than 0x28(%rsp) for cpu, I think
asoc->stream.si->make_datafrag() is even better. No ?

From: Xin Long

> Sent: 08 December 2017 16:18

> 

...
> >> Alternatively you could preform the dereference in two steps (i.e. declare an si

> >> pointer on the stack and set it equal to asoc->stream.si, then deref

> >> si->make_datafrag at call time.  That will at least give the compiler an

> >> opportunity to preload the first pointer.


You want to save the function pointer itself.

...
> Another small difference:

>   as you can see, comparing to (X), (Y) is using 0x28(%rsp) in the loop,

>   instead of %r13.

> 

> So that's what I can see from the related generated code.

> If 0x848(%r13) is not worse than 0x28(%rsp) for cpu, I think

> asoc->stream.si->make_datafrag() is even better. No ?


That code must have far too many life local variables.
Otherwise there's be a caller saved register available.

	David

On Sat, Dec 9, 2017 at 12:22 AM, David Laight <David.Laight@aculab.com> wrote:
> From: Xin Long
>> Sent: 08 December 2017 16:18
>>
> ...
>> >> Alternatively you could preform the dereference in two steps (i.e. declare an si
>> >> pointer on the stack and set it equal to asoc->stream.si, then deref
>> >> si->make_datafrag at call time.  That will at least give the compiler an
>> >> opportunity to preload the first pointer.
>
> You want to save the function pointer itself.
>
> ...
>> Another small difference:
>>   as you can see, comparing to (X), (Y) is using 0x28(%rsp) in the loop,
>>   instead of %r13.
>>
>> So that's what I can see from the related generated code.
>> If 0x848(%r13) is not worse than 0x28(%rsp) for cpu, I think
>> asoc->stream.si->make_datafrag() is even better. No ?
>
> That code must have far too many life local variables.
> Otherwise there's be a caller saved register available.
>
Hi, David, Sorry,  I'm not sure we're worrying about the cpu cost or
codes style now ?

For cpu cost,  I think 0x848(%r13) operation must be better than the
generated code of if-else.

For the codes style, comparing to the if-else, I think this one is
more readable. (ignore extendible stuff first, as probably no more
new type of data chunk).

From: Xin Long ...

> Hi, David, Sorry,  I'm not sure we're worrying about the cpu cost or

> codes style now ?

> 

> For cpu cost,  I think 0x848(%r13) operation must be better than the

> generated code of if-else.


Nope - the call xxx(%ryyy) is likely to be a data cache miss and a complete
cpu pipeline stall.

The conditional will be a data cache hit and the code (for one branch)
will be prefetched and speculatively executed.

Some very modern cpu might manage to predict indirect jumps, but for
most it is a full pipeline stall.

	David

On Sat, Dec 9, 2017 at 1:29 AM, David Laight <David.Laight@aculab.com> wrote:
> From: Xin Long ...
>> Hi, David, Sorry,  I'm not sure we're worrying about the cpu cost or
>> codes style now ?
>>
>> For cpu cost,  I think 0x848(%r13) operation must be better than the
>> generated code of if-else.
>
> Nope - the call xxx(%ryyy) is likely to be a data cache miss and a complete
> cpu pipeline stall.
>
> The conditional will be a data cache hit and the code (for one branch)
> will be prefetched and speculatively executed.
>
> Some very modern cpu might manage to predict indirect jumps, but for
> most it is a full pipeline stall.
Thanks for the CPU information.

The thing is with if-else can't avoid xxx(%ryyy) in this case, as Marcelo
said above. It seems if-else will just be a extra cost compare to this one.

On Fri, Dec 08, 2017 at 11:08:56AM -0500, Neil Horman wrote:
> On Fri, Dec 08, 2017 at 04:04:58PM +0000, David Laight wrote:
> > From: Marcelo Ricardo Leitner
> > > Sent: 08 December 2017 16:00
> > ...
> > > > Is it worth replacing the si struct with an index/enum value, and indexing an
> > > > array of method pointer structs?  That would save you at least one dereference.
> > > 
> > > Hmmm, maybe, yes. It would be like
> > > sctp_stream_interleave[asoc->stream.si].make_datafrag(...)
> > 
> > If you only expect 2 choices then an if () is likely
> > to produce better code that the above.
> > 
> > The actual implementation can be hidden inside a #define
> > or static inline function.
> > 
> Thats the real question though, will we expect more than two interleaving
> strategies?  Currently its a boolean operation so the answer seems like yes, but
> is there a possiblity of a biased interleaving, or other worthwhile algorithm?

For the chunk format, I don't think so. It would require another RFC
update.
For other possibilities on having a 3rd choice in there, I also don't
think so. Stream scheduling is handled apart from it and rx buffer
stuff is being covered by it now, don't see how we could have a 3rd
option without another chunk format.

But that said, I don't think the macro or even inline wrappers are as
clear as the struct with function pointers here and in some cases it
even won't avoid the second deref. I wouldn't like to compromise code
readability and OO because of 1 fetch, specially considering that this
will be barely noticeable.

Neil's idea on using the array of structs and indexing on it is nice.
Saves a deref and keeps the abstractions without inserting too much
noise on it. Plus, it also allows replacing the struct pointer in
sctp_stream with a bit. If then placed before the union in there, we
can save up to the whole pointer. Looks like a good compromise to me.

  Marcelo