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? > > > /* 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. 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. 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? 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