Re: [etherlab-dev] Pre-announcement: unofficial patchset update (Gavin Lambert)
On 7 May 2017 20:33, quoth Graeme Foot: > I don't use ec_master_set_send_interval() or --enable-hrtimer since my masters operational thread > has always run at 10ms (100Hz) anyway. (I probably should so will look at adding that in at some > stage.) My Linux kernel is configured to run at 100Hz and the master thread is not realtime so is > scheduled by the Linux scheduler (RTAI). Because Linux is set to 100Hz, it only runs the masters > operation thread once every 10ms. > > Prior to your SDO patch, ec_fsm_master_action_process_sdo() was being called by the masters > fsm, but from its idle state. So it would only be called after all other processing and > housekeeping was complete and was only being fired approx once every 800ms on my setup > with 50 odd slaves. After the patch ec_master_exec_slave_fsms() is now called every time > the masters operational thread fires. All good except that on my system that is still only once > every 10ms. Right, but what I was saying is that prior to my patch it would actually service the requests faster than 10ms even on your system due to the way the re-scheduling is done (the master isn't idle until it finishes any outstanding requests, so it calls schedule() instead of schedule_timeout(1) -- ie. if there's no other work for the kernel to do it will reschedule immediately instead of waiting for the next 10ms time slice). After my patch the master is idle even while requests are in progress, so the condition it checks is no longer sufficient and it calls schedule_timeout(1) too soon, making it slower than it should be. I didn't notice this regression because I *am* using --enable-hrtimer, which does not have the same issue. So what I was suggesting is that *you* try using --enable-hrtimer, which I'm reasonably certain will solve that performance issue without needing to try to exec slave FSMs from the realtime context. If you can't use --enable-hrtimer for some reason, then the most likely solution to the above issue is to find the two lines where it checks for ec_fsm_master_idle (in ec_master_idle_thread and ec_master_operation_thread) and change the condition from this: if (ec_fsm_master_idle(>fsm)) { to this: if (ec_fsm_master_idle(>fsm) && !master->fsm_exec_count) { This is just air code and I haven't tested it, but it seems reasonably likely to solve the issue and restore performance without --enable-hrtimer to pre-patch levels or better. Though there might be a risk that it will make the kernel do some busy-waiting in some cases, though that shouldn't bother an RTAI application. > So my new patch allows ec_master_exec_slave_fsms() to be called from my realtime context. As > you pointed out the master_sem lock would cause a deadlock, so I don't use it. Because I don't > use the lock I have instead added some flags to track whether it is currently safe to make the > ec_master_exec_slave_fsms() call. It's generally just the rescan thats a problem. I haven't looked at your patch in detail, but it makes me nervous to pull code outside of a lock like that; there are a lot of data structures that it protects, and some of them might be more subtle than rescan. Also, while this probably isn't a problem with RTAI (since it can pre-empt the Linux kernel), this API probably would be unsafe to use with regular kernel or userspace code due to the inverse problem -- what if the app code is in the middle of executing ec_master_exec_slave_fsms() when the master thread decides to start a rescan (or otherwise mutate data structures it depends on)? > I don't know if the patch will be useful for anyone else, but is useful if Linux is configured for > 100Hz. It may also be useful on short cycle time systems, e.g. 100 - 250us cycle times, > where you want to process the SDO's faster. Even if Linux is set to 1000Hz is will only > schedule the master operational thread at 1ms. The master thread may also be delayed if > the Linux side gets some heavy CPU usage. SDOs by design are intended to be slower-than-cycle tasks. They're for occasional configuration, diagnostic, or slow acyclic tasks, not for rapid activity, so if you're trying to get 1ms or higher response rates out of them, you're probably doing it wrong. (Recommended timeouts for SDO tasks are generally measured in *seconds*, not milliseconds.) ___ etherlab-dev mailing list etherlab-dev@etherlab.org http://lists.etherlab.org/mailman/listinfo/etherlab-dev
Re: [etherlab-dev] Pre-announcement: unofficial patchset update (Gavin Lambert)
> > Mere moments ago, quoth I: > > On 28 April 2017 11:59, quoth Graeme Foot: > > > 2) Your 0054 patch successfully moved the slave sdo request processing > > > from the master fsm's idle state to be called every cycle of the master > > > thread. > > > However, I have my Linux environment set to run at 100Hz, so this > > > thread would only fire once every 10ms. Each SDO read/write request > > > would take approx. 30ms to complete. I didn't want to change Linux to > > > run at 1000Hz so I > > > created patch "etherlabmaster-0010- > > > allow_app_to_process_sdo_requests_from_realtime.patch". > > > > Maybe I'm missing something about how the RTAI version works, but all I > did > > was to move the logic from inside fsm_master to inside fsm_slave; AFAIK > > both of these execute on the same thread (either the master IDLE or master > > OPERATION thread). This is independent of the application realtime loop > > either way, so this should not have changed how frequently they run; it > just > > allows multiple slave requests to run in parallel rather than forcing them > to > > execute sequentially, so it should be a net performance gain. > > > > Though it does seem reasonable in your use case to want to run the slave > > FSMs more often. I don't see how you could do that safely, though -- you > > can't run ec_master_exec_slave_fsms outside of the master_sem lock, and > > your RTAI task might interrupt Linux while it's still holding that lock, > which will > > deadlock your RTAI task. > > Are you configuring with --enable-hrtimer? After having a quick look at the > code, I can see a potential performance degradation from the patch if you > aren't using it (or did enable it but didn't call > ec_master_set_send_interval with an appropriate value after activating the > master). Perhaps you could try enabling that instead of using your patch > and see if it helps? > Hi, I don't use ec_master_set_send_interval() or --enable-hrtimer since my masters operational thread has always run at 10ms (100Hz) anyway. (I probably should so will look at adding that in at some stage.) My Linux kernel is configured to run at 100Hz and the master thread is not realtime so is scheduled by the Linux scheduler (RTAI). Because Linux is set to 100Hz, it only runs the masters operation thread once every 10ms. Prior to your SDO patch, ec_fsm_master_action_process_sdo() was being called by the masters fsm, but from its idle state. So it would only be called after all other processing and housekeeping was complete and was only being fired approx once every 800ms on my setup with 50 odd slaves. After the patch ec_master_exec_slave_fsms() is now called every time the masters operational thread fires. All good except that on my system that is still only once every 10ms. So my new patch allows ec_master_exec_slave_fsms() to be called from my realtime context. As you pointed out the master_sem lock would cause a deadlock, so I don't use it. Because I don't use the lock I have instead added some flags to track whether it is currently safe to make the ec_master_exec_slave_fsms() call. It's generally just the rescan thats a problem. I don't know if the patch will be useful for anyone else, but is useful if Linux is configured for 100Hz. It may also be useful on short cycle time systems, e.g. 100 - 250us cycle times, where you want to process the SDO's faster. Even if Linux is set to 1000Hz is will only schedule the master operational thread at 1ms. The master thread may also be delayed if the Linux side gets some heavy CPU usage. Thanks for thinking about it. I like getting feedback for my patches as its easy to overlook potential bugs or alternate solutions which can be better. Graeme. ___ etherlab-dev mailing list etherlab-dev@etherlab.org http://lists.etherlab.org/mailman/listinfo/etherlab-dev
Re: [etherlab-dev] Pre-announcement: unofficial patchset update (Gavin Lambert)
Mere moments ago, quoth I: > On 28 April 2017 11:59, quoth Graeme Foot: > > 2) Your 0054 patch successfully moved the slave sdo request processing > > from the master fsm's idle state to be called every cycle of the master > > thread. > > However, I have my Linux environment set to run at 100Hz, so this > > thread would only fire once every 10ms. Each SDO read/write request > > would take approx. 30ms to complete. I didn't want to change Linux to > > run at 1000Hz so I > > created patch "etherlabmaster-0010- > > allow_app_to_process_sdo_requests_from_realtime.patch". > > Maybe I'm missing something about how the RTAI version works, but all I did > was to move the logic from inside fsm_master to inside fsm_slave; AFAIK > both of these execute on the same thread (either the master IDLE or master > OPERATION thread). This is independent of the application realtime loop > either way, so this should not have changed how frequently they run; it just > allows multiple slave requests to run in parallel rather than forcing them to > execute sequentially, so it should be a net performance gain. > > Though it does seem reasonable in your use case to want to run the slave > FSMs more often. I don't see how you could do that safely, though -- you > can't run ec_master_exec_slave_fsms outside of the master_sem lock, and > your RTAI task might interrupt Linux while it's still holding that lock, which will > deadlock your RTAI task. Are you configuring with --enable-hrtimer? After having a quick look at the code, I can see a potential performance degradation from the patch if you aren't using it (or did enable it but didn't call ec_master_set_send_interval with an appropriate value after activating the master). Perhaps you could try enabling that instead of using your patch and see if it helps? ___ etherlab-dev mailing list etherlab-dev@etherlab.org http://lists.etherlab.org/mailman/listinfo/etherlab-dev
Re: [etherlab-dev] Pre-announcement: unofficial patchset update (Gavin Lambert)
On 28 April 2017 11:59, quoth Graeme Foot: > First off, I also prefer #3. I use buildroot to create my Linux environment and > buildroot applies patches in alphabetical order (at least in my version which is > now pretty old), so the number at the front is important. Buildroot also > requires that the patch starts with the name of the package (and optionally a > revision number), but that is easy for me to prefix. FWIW, it looks like buildroot does follow a series file if one is present, so you could just take the series file that's supplied with the patchset and comment or delete from it the patches you're uninterested in, and add additional ones of your own. > When I tried to use your patchset (and the EtherCAT revision they were for) > the computer would freeze just after starting my application and going > realtime. We use RTAI and I read your notes re that it wasn't tested with > RTAI. I didn't have much time to look into problem but I suspect there may > have been a lock that ended up in a call from the realtime context that was > blocked due to be held in the master thread. I suspect this is probably either patch 0007, patch 0011, or patch 0030. If you do get a chance to look into it, it'd be good to confirm that. I'm considering wrapping one or more of these in a configure --enable (or otherwise disabling them when RTDM is enabled). > I ended up cherry picking the changes I needed (patch 0054 (sdo requests > via slave fsm) and 0038 (sdo write request)). That could be a little dangerous; the 005x patches are highly dependent on prior patches and it's probably very bad to run 0054 without 0050-0053. > As to other potential patches (Note: my patches are against 2526 > (2eff7c993a63) on the stable-1.5 branch): Thanks, I'll look at including these too. > 2) Your 0054 patch successfully moved the slave sdo request processing from > the master fsm's idle state to be called every cycle of the master thread. > However, I have my Linux environment set to run at 100Hz, so this thread > would only fire once every 10ms. Each SDO read/write request would take > approx. 30ms to complete. I didn't want to change Linux to run at 1000Hz so I > created patch "etherlabmaster-0010- > allow_app_to_process_sdo_requests_from_realtime.patch". Maybe I'm missing something about how the RTAI version works, but all I did was to move the logic from inside fsm_master to inside fsm_slave; AFAIK both of these execute on the same thread (either the master IDLE or master OPERATION thread). This is independent of the application realtime loop either way, so this should not have changed how frequently they run; it just allows multiple slave requests to run in parallel rather than forcing them to execute sequentially, so it should be a net performance gain. Though it does seem reasonable in your use case to want to run the slave FSMs more often. I don't see how you could do that safely, though -- you can't run ec_master_exec_slave_fsms outside of the master_sem lock, and your RTAI task might interrupt Linux while it's still holding that lock, which will deadlock your RTAI task. ___ etherlab-dev mailing list etherlab-dev@etherlab.org http://lists.etherlab.org/mailman/listinfo/etherlab-dev
Re: [etherlab-dev] Pre-announcement: unofficial patchset update (Gavin Lambert)
ffort has been much appreciated. > > > Regards, > Graeme Foot > > > -----Original Message- > From: etherlab-dev [mailto:etherlab-dev-boun...@etherlab.org] On Behalf Of > Knud Baastrup > Sent: Thursday, 27 April 2017 11:11 p.m. > To: Gavin Lambert <gav...@compacsort.com> > Cc: etherlab-dev@etherlab.org > Subject: Re: [etherlab-dev] Pre-announcement: unofficial patchset update > (Gavin Lambert) > > Hi Gavin, > > I am also in favor of #3 and suggest that naming is "auto generated" based on > the first line of the commit message like done with git-format-patch. > > "By default, each output file is numbered sequentially from 1, and uses the > first line of the commit message (massaged for pathname safety) as the > filename." (https://git-scm.com/docs/git-format-patch) > > > It could be great if it was possible to maintain backwards compatibility in > the patch-serie or alternatively ensure that breaking patches are put on top > or in a separate serie. > > > BR, Knud > > > -Original Message- > From: etherlab-dev [mailto:etherlab-dev-boun...@etherlab.org] On Behalf Of > etherlab-dev-requ...@etherlab.org > Sent: 27. april 2017 12:00 > To: etherlab-dev@etherlab.org > Subject: etherlab-dev Digest, Vol 93, Issue 3 > > Send etherlab-dev mailing list submissions to > etherlab-dev@etherlab.org > > To subscribe or unsubscribe via the World Wide Web, visit > http://lists.etherlab.org/mailman/listinfo/etherlab-dev > or, via email, send a message with subject or body 'help' to > etherlab-dev-requ...@etherlab.org > > You can reach the person managing the list at > etherlab-dev-ow...@etherlab.org > > When replying, please edit your Subject line so it is more specific than "Re: > Contents of etherlab-dev digest..." > > > Today's Topics: > >1. Pre-announcement: unofficial patchset update (Gavin Lambert) > > > -- > > Message: 1 > Date: Thu, 27 Apr 2017 19:31:21 +1200 > From: Gavin Lambert <gav...@compacsort.com> > To: <etherlab-dev@etherlab.org> > Subject: [etherlab-dev] Pre-announcement: unofficial patchset update > Message-ID: <01d2bf28$47333120$d5999360$@compacsort.com> > Content-Type: text/plain; charset="us-ascii" > > Hi all, > > I've recently been doing some more work on my local Etherlab-related code, > and as a result I'm planning to "shortly" (by which I mean still probably a > few weeks away) release a new version of my default-branch unofficial > patchset. > > I thought it would be a good idea to let everyone know in advance both to > gather any new patches people might want to add (or feedback on or suggested > changes to existing patches), but also since this is the first update since > publishing it as a repository I'd like to know people's preferences with > regard to re-ordering patches from the existing set. For example, I could: > > 1. Retain existing patches exactly as is (barring fuzz updates) and only add > new patches (modifications to existing patches are added as a new patch). > 2. Retain existing patches with the same numbering but allow both new patches > (with strictly higher numbers) and modifying existing patches. > 3. Renumber existing patches as needed to insert new patches in a logical > place (either grouping patches by related function or putting the simplest > patches first so they have fewer dependencies and are thus hopefully easier > to get included into upstream). > 4. Abandon the idea of numbered patches entirely and just rely on consistent > names plus the series file to maintain order. (Thus also allowing new > patches to be inserted wherever seems sensible.) > > If I don't get any feedback, I'm probably inclined towards #3 at this point, > but I might go to #4 if it looks tricky to maintain patch history properly > with #3. I could be persuaded towards #2 (though it's not my preference) but > am disinclined towards #1. Or maybe there's some alternate method I haven't > considered. > > I might see if I can group related patches into subdirectories -- I know > quilt/Debian patchsets support this, though I'm not sure about HG patchsets. > > (FYI, I've already made a note of the patch updates Knud Baastrup submitted > in December, and a few other patches posted to the list since then, though I > haven't integrated them yet.) > > Regards, > Gavin Lambert > > > > > -- > > Subject: Digest Footer > > ___ > etherlab-dev mailing list > etherl
Re: [etherlab-dev] Pre-announcement: unofficial patchset update (Gavin Lambert)
Hi, I just had a play with your patchset the other day to try to resolve some very slow SDO read/write timings. First off, I also prefer #3. I use buildroot to create my Linux environment and buildroot applies patches in alphabetical order (at least in my version which is now pretty old), so the number at the front is important. Buildroot also requires that the patch starts with the name of the package (and optionally a revision number), but that is easy for me to prefix. I would prefer breaking change patches to be last in the list if possible. When I tried to use your patchset (and the EtherCAT revision they were for) the computer would freeze just after starting my application and going realtime. We use RTAI and I read your notes re that it wasn't tested with RTAI. I didn't have much time to look into problem but I suspect there may have been a lock that ended up in a call from the realtime context that was blocked due to be held in the master thread. I ended up cherry picking the changes I needed (patch 0054 (sdo requests via slave fsm) and 0038 (sdo write request)). FYI: We are still using Linux kernel 2.6.32.11 and had a compilation issue with one of the patches due to header file differences. Unfortunately I seem to have deleted the info as to what it was. As to other potential patches (Note: my patches are against 2526 (2eff7c993a63) on the stable-1.5 branch): 1) RTAI has issues with fprintf calls from the realtime context. It should instead use rt_printk. My patch "etherlabmaster-0001-replace_fprintf_calls_with_EC_PRINT_ERR.patch" defines a macro that replaces fprintf calls if USE_RTDM is defined. 2) Your 0054 patch successfully moved the slave sdo request processing from the master fsm's idle state to be called every cycle of the master thread. However, I have my Linux environment set to run at 100Hz, so this thread would only fire once every 10ms. Each SDO read/write request would take approx. 30ms to complete. I didn't want to change Linux to run at 1000Hz so I created patch "etherlabmaster-0010-allow_app_to_process_sdo_requests_from_realtime.patch". This patch optionally allows the slave request processing to be called from within my application in the realtime thread. I added two new functions, one to set whether slave request processing is expected to be processed by the master thread as usual, or by your app. The second function needs to be called periodically by your app. Note: if the master is idle (ie your app is not running) then sdo request processing will be handled as normal. In my app I process the slave requests once every 2ms (every second realtime cycle) at the end of the realtime cycle. At this rate, SDO read/writes are handled within 6ms (a couple of extra ms due to other application timings). 3) One last patch you may be interested in. I was having problems with my distributed clock setup. Some changes I had made a while ago caused my system to have a very/very slow drift (approx 1ms over 63 days). Every now and then our machines would start running roughly which a repower would resolve. It turned out to be due to the machines not having been repowered for a few weeks and the drift would eventually cause the EtherCAT frame to be sent out at about the time of the DC sync events in the slaves and due to jitter the frames randomly falling either side of the event. To help diagnose this I added a function to queue and read the dc reference slave clocks 64bit time. Effectively it does nothing except put the whole 64bit clock time into the EtherCAT frame so that wireshark has a long running time reference (rather than just the 32bit low part that rolls over every 4 odd seconds). Once I put that in, the drift (even though very small) became very obvious. (etherlabmaster-0008-read_reference_slave_clock_64bit_time.patch) It takes a fair bit of time creating and maintaining patches so everyone's effort has been much appreciated. Regards, Graeme Foot -Original Message- From: etherlab-dev [mailto:etherlab-dev-boun...@etherlab.org] On Behalf Of Knud Baastrup Sent: Thursday, 27 April 2017 11:11 p.m. To: Gavin Lambert <gav...@compacsort.com> Cc: etherlab-dev@etherlab.org Subject: Re: [etherlab-dev] Pre-announcement: unofficial patchset update (Gavin Lambert) Hi Gavin, I am also in favor of #3 and suggest that naming is "auto generated" based on the first line of the commit message like done with git-format-patch. "By default, each output file is numbered sequentially from 1, and uses the first line of the commit message (massaged for pathname safety) as the filename." (https://git-scm.com/docs/git-format-patch) It could be great if it was possible to maintain backwards compatibility in the patch-serie or alternatively ensure that breaking patches are put on top or in a separate serie. BR, Knud -Original Message
Re: [etherlab-dev] Pre-announcement: unofficial patchset update (Gavin Lambert)
Hi Gavin, I am also in favor of #3 and suggest that naming is "auto generated" based on the first line of the commit message like done with git-format-patch. "By default, each output file is numbered sequentially from 1, and uses the first line of the commit message (massaged for pathname safety) as the filename." (https://git-scm.com/docs/git-format-patch) It could be great if it was possible to maintain backwards compatibility in the patch-serie or alternatively ensure that breaking patches are put on top or in a separate serie. BR, Knud -Original Message- From: etherlab-dev [mailto:etherlab-dev-boun...@etherlab.org] On Behalf Of etherlab-dev-requ...@etherlab.org Sent: 27. april 2017 12:00 To: etherlab-dev@etherlab.org Subject: etherlab-dev Digest, Vol 93, Issue 3 Send etherlab-dev mailing list submissions to etherlab-dev@etherlab.org To subscribe or unsubscribe via the World Wide Web, visit http://lists.etherlab.org/mailman/listinfo/etherlab-dev or, via email, send a message with subject or body 'help' to etherlab-dev-requ...@etherlab.org You can reach the person managing the list at etherlab-dev-ow...@etherlab.org When replying, please edit your Subject line so it is more specific than "Re: Contents of etherlab-dev digest..." Today's Topics: 1. Pre-announcement: unofficial patchset update (Gavin Lambert) -- Message: 1 Date: Thu, 27 Apr 2017 19:31:21 +1200 From: Gavin LambertTo: Subject: [etherlab-dev] Pre-announcement: unofficial patchset update Message-ID: <01d2bf28$47333120$d5999360$@compacsort.com> Content-Type: text/plain; charset="us-ascii" Hi all, I've recently been doing some more work on my local Etherlab-related code, and as a result I'm planning to "shortly" (by which I mean still probably a few weeks away) release a new version of my default-branch unofficial patchset. I thought it would be a good idea to let everyone know in advance both to gather any new patches people might want to add (or feedback on or suggested changes to existing patches), but also since this is the first update since publishing it as a repository I'd like to know people's preferences with regard to re-ordering patches from the existing set. For example, I could: 1. Retain existing patches exactly as is (barring fuzz updates) and only add new patches (modifications to existing patches are added as a new patch). 2. Retain existing patches with the same numbering but allow both new patches (with strictly higher numbers) and modifying existing patches. 3. Renumber existing patches as needed to insert new patches in a logical place (either grouping patches by related function or putting the simplest patches first so they have fewer dependencies and are thus hopefully easier to get included into upstream). 4. Abandon the idea of numbered patches entirely and just rely on consistent names plus the series file to maintain order. (Thus also allowing new patches to be inserted wherever seems sensible.) If I don't get any feedback, I'm probably inclined towards #3 at this point, but I might go to #4 if it looks tricky to maintain patch history properly with #3. I could be persuaded towards #2 (though it's not my preference) but am disinclined towards #1. Or maybe there's some alternate method I haven't considered. I might see if I can group related patches into subdirectories -- I know quilt/Debian patchsets support this, though I'm not sure about HG patchsets. (FYI, I've already made a note of the patch updates Knud Baastrup submitted in December, and a few other patches posted to the list since then, though I haven't integrated them yet.) Regards, Gavin Lambert -- Subject: Digest Footer ___ etherlab-dev mailing list etherlab-dev@etherlab.org http://lists.etherlab.org/mailman/listinfo/etherlab-dev -- End of etherlab-dev Digest, Vol 93, Issue 3 *** ___ etherlab-dev mailing list etherlab-dev@etherlab.org http://lists.etherlab.org/mailman/listinfo/etherlab-dev