On Wed, 26 Apr 2023 at 11:55, Maciek Machnikowski <mac...@machnikowski.net> wrote:
> On 4/21/2023 6:46 PM, Erez wrote: > > > > > > On Fri, 21 Apr 2023 at 17:27, Maciek Machnikowski > > <mac...@machnikowski.net <mailto:mac...@machnikowski.net>> wrote: > > > > On 4/21/2023 1:25 PM, Erez wrote: > > > > > > > > > On Thu, 20 Apr 2023 at 19:01, Maciek Machnikowski > > > <mac...@machnikowski.net <mailto:mac...@machnikowski.net> > > <mailto:mac...@machnikowski.net <mailto:mac...@machnikowski.net>>> > > wrote: > > > > > > Add option to run callback that when the PMC agent receives > > > signaling messages. > > > > > > Signed-off-by: Maciek Machnikowski <mac...@machnikowski.net > > <mailto:mac...@machnikowski.net> > > > <mailto:mac...@machnikowski.net <mailto: > mac...@machnikowski.net>>> > > > --- > > > pmc_agent.c | 21 +++++++++++++++++++-- > > > pmc_agent.h | 7 +++++++ > > > 2 files changed, 26 insertions(+), 2 deletions(-) > > > > > > diff --git a/pmc_agent.c b/pmc_agent.c > > > index 62d1a86..ef4e1bb 100644 > > > --- a/pmc_agent.c > > > +++ b/pmc_agent.c > > > @@ -42,6 +42,7 @@ struct pmc_agent { > > > bool dds_valid; > > > int leap; > > > int pmc_ds_requested; > > > + bool signaling_cb_ena; > > > bool stay_subscribed; > > > int sync_offset; > > > int utc_offset_traceable; > > > @@ -127,6 +128,7 @@ static int run_pmc(struct pmc_agent *node, > int > > > timeout, int ds_id, > > > #define N_FD 1 > > > struct pollfd pollfd[N_FD]; > > > int cnt, res; > > > + bool skip_cb; > > > > > > while (1) { > > > pollfd[0].fd = pmc_get_transport_fd(node->pmc); > > > @@ -178,9 +180,18 @@ static int run_pmc(struct pmc_agent > > *node, int > > > timeout, int ds_id, > > > node->pmc_ds_requested = 0; > > > return RUN_PMC_NODEV; > > > } > > > - if (res <= 0 || > > > + > > > + /* Skip callback if message is not management > */ > > > + skip_cb = (res <= 0) ? true : false; > > > + > > > + /* Run the callback on signaling messages if > > > configured */ > > > + if (node->signaling_cb_ena && (msg_type(*msg) > == > > > SIGNALING)) { > > > > > > > > > I would add res == 0, to save time here. > > > > > > You can have > > > if (res < 0) { > > > skip_cb = false; > > > } else if(res == 0) { > > > if (node->signaling_cb_ena && (msg_type(*msg) == > > > SIGNALING)) { > > > skip_cb = true; > > > } else { > > > skip_cb = false; > > > } > > > } else { > > > skip_cb = true; > > > } > > > > > > This code saves you from checking 'node->signaling_cb_ena' if res > > is not 0. > > > 'res' is a local variable, 'node->signaling_cb_ena' is referred > > through> a pointer, so its fetch time is much bigger. > > > > This is not true. It compiles to a single ASM line even without any > > optimizations enabled: > > > > > > This looks like arm64. There are other CPUs beside ARM. > > And in assembler it is hard to "see" how long it takes to load > > 'signaling_cb_ena' from node unless it is in the memory cache. > > It's similar on x86. > /* Run the callback on signaling messages if configured */ > if (res == 0 && node->signaling_cb_ena && > 475: 83 7d f4 00 cmpl $0x0,-0xc(%rbp) > 479: 75 24 jne 49f <run_pmc+0x1e0> > >47b: 48 8b 45 d8 mov -0x28(%rbp),%rax > >47f: 0f b6 40 30 movzbl 0x30(%rax),%eax > >483: 84 c0 test %al,%al > 485: 74 18 je 49f <run_pmc+0x1e0> > msg_type(*msg) == SIGNALING) { > 487: 48 8b 45 c8 mov -0x38(%rbp),%rax > 48b: 48 8b 00 mov (%rax),%rax > 48e: 48 89 c7 mov %rax,%rdi > 491: e8 39 fc ff ff callq cf <msg_type> > > Do you know any architecture that's different than this? > There are about 12 different architectures, not to mention 32 and 64 bit and old 8 and 16 bits. But this group is not about machine code and assembler. I suggest we leave it out. This argument does not fly anywhere. > > > > > > > /* Skip callback if message is not management */ > > skip_cb = (res <= 0) ? true : false; > > 54c: b94047e0 ldr w0, [sp, #68] > > 550: 7100001f cmp w0, #0x0 > > 554: 1a9fc7e0 cset w0, le > > 558: 39013fe0 strb w0, [sp, #79] > > /* Run the callback on signaling messages if configured */ > > if (node->signaling_cb_ena && (msg_type(*msg) == > > SIGNALING)) { > > 55c: f94017e0 ldr x0, [sp, #40] > > >560: 3940c000 ldrb w0, [x0, #48] > > 564: 7100001f cmp w0, #0x0 > > 568: 540000e0 b.eq 584 <run_pmc+0x210> // b.none > > 56c: f9400fe0 ldr x0, [sp, #24] > > 570: f9400000 ldr x0, [x0] > > 574: 97fffeea bl 11c <msg_type> > > 578: 7100301f cmp w0, #0xc > > 57c: 54000041 b.ne <http://b.ne> 584 <run_pmc+0x210> > > // b.any > > skip_cb = false; > > 580: 39013fff strb wzr, [sp, #79] > > } > > > > > > Adding a check for res adds 3 more ASM instructions: > > > > > > But save loading `signaling_cb_ena` if you mainly use > management messages! > > > > > > > > /* Run the callback on signaling messages if > > configured */ > > if (res == 0 && node->signaling_cb_ena && > > >55c: b94047e0 ldr w0, [sp, #68] > > >560: 7100001f cmp w0, #0x0 > > >564: 54000161 b.ne <http://b.ne> 590 <run_pmc+0x21c> > > // b.any > > 568: f94017e0 ldr x0, [sp, #40] > > 56c: 3940c000 ldrb w0, [x0, #48] > > 570: 7100001f cmp w0, #0x0 > > 574: 540000e0 b.eq 590 <run_pmc+0x21c> // b.none > > msg_type(*msg) == SIGNALING) { > > 578: f9400fe0 ldr x0, [sp, #24] > > 57c: f9400000 ldr x0, [x0] > > 580: 97fffee7 bl 11c <msg_type> > > if (res == 0 && node->signaling_cb_ena && > > 584: 7100301f cmp w0, #0xc > > 588: 54000041 b.ne <http://b.ne> 590 <run_pmc+0x21c> > > // b.any > > skip_cb = false; > > 58c: 39013fff strb wzr, [sp, #79] > > } > > > > Thanks, > > Maciek > > > > > > Again you can simply reuse `res` and have a small code and also faster > code. > > I do not understand why you are afraid of using reuse `res`. > > > > I only point on optimization. I assume that most cases are > > management messages, and testing the signaling flag for all packets does > > not help. > > > > If you want a better solution, you replace in run_pmc() : > > > > type = msg_type(msg); > > switch (type) { > > case SIGNALING: > > res = node->signaling_cb_ena ? 2 : 0; > > break; > > case MANAGEMENT: > > res = is_msg_mgt(msg); > > break; > > default: > > break > > } > > > > And remove the 'msg_type(msg) != MANAGEMENT' from is_msg_mgt(). > > Optimizing run_pmc is beyond the scope of my patch. I'll add a check for > res==0 before checking node->signaling_cb_ena, but I hardly believe > it'll have any performance benefit. > It is your call. As you change the code, you can also optimize your change. Nothing is holy with the current implementation. I understand you want to do "minimum". But the current code is also optimized code. > > If you run the run_pmc infrequently, this check will not matter, as it > takes only a couple of ns to read from memory. You'll spend much more > time on function calls preceding this check. > Yes, this code does not run much. This is a more basic question. Do optimize only a hard real time code. And leave all the rest, heavy and badly optimized? Again, I understand why you think minimal changes are more pretty. It is your patch, you and Richard decide. I only share my opinion. > > If you call run_pmc constantly, then this location will most likely be > cached, as it holds a static value, and it will still not negatively > impact the performance. To see any difference you'd need to hit a ton of > messages/second - is there any use case that requires it? > Again, not all architectures are using catch memory. Please do not make assumptions. Same as machine code, this is not the group. And yes, this is not hard real time. I do not ask for a very complicated optimization. Just write good code, please. Erez > > Thanks > Maciek > > > > > Assembler rarely helps to understand speed and size of code. > > Especially code that runs on multiple CPUs as free software. > > > > > > > > > > > > Erez > > > > > > > > > > > This code is bigger, but you do not want to reuse the 'res' > variable. > > > > > > > > > > > > + skip_cb = false; > > > + } > > > + > > > + if (skip_cb || > > > node->recv_subscribed(node->recv_context, > > *msg, > > > ds_id) || > > > - management_tlv_id(*msg) != ds_id) { > > > + (res == 1 && management_tlv_id(*msg) != > > ds_id)) { > > > > > > > > > This part is OK. > > > > > > Erez > > > > > > > > > > > > msg_put(*msg); > > > *msg = NULL; > > > continue; > > > @@ -430,3 +441,9 @@ bool pmc_agent_utc_offset_traceable(struct > > > pmc_agent *agent) > > > { > > > return agent->utc_offset_traceable; > > > } > > > + > > > +void pmc_agent_enable_signaling_cb(struct pmc_agent *agent, > bool > > > enable) > > > +{ > > > + agent->signaling_cb_ena = enable; > > > +} > > > + > > > diff --git a/pmc_agent.h b/pmc_agent.h > > > index 2fb1cc8..9ae37f7 100644 > > > --- a/pmc_agent.h > > > +++ b/pmc_agent.h > > > @@ -170,4 +170,11 @@ int pmc_agent_update(struct pmc_agent > > *agent); > > > */ > > > bool pmc_agent_utc_offset_traceable(struct pmc_agent *agent); > > > > > > +/** > > > + * Enables or disables callback on signaling messages > > > + * @param agent Pointer to a PMC instance obtained via @ref > > > pmc_agent_create(). > > > + * @param enable - if set to true, callback will be called on > > > signaling msgs > > > + */ > > > +void pmc_agent_enable_signaling_cb(struct pmc_agent *agent, > bool > > > enable); > > > + > > > #endif > > > -- > > > 2.30.2 > > > > > >
_______________________________________________ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
