(corrected some typos in my previous mail) I?ll begin the work on adding support for recursive lock to oc_mutex this weekend. As for Dave?s concerns below, I will not change the default behavior and will make sure to document the new API to express these concerns.
By the way I?m not inventing the need for a recursive mutex. I?m just moving the existing one in the C++ layer (aka. the ?csdk? lock in OCPlatform) to the stack in order to fix the 10ms application loop as outlined in IOT-1828. Please let me know if you have any questions or concerns. Thanks, Way From: iotivity-dev-bounces at lists.iotivity.org [mailto:iotivity-dev-boun...@lists.iotivity.org] On Behalf Of Way Vadhanasin via iotivity-dev Sent: Wednesday, April 5, 2017 10:28 AM To: Dave Thaler <dthaler at microsoft.com>; jw0213.jung at samsung.com; Soemin Tjong <stjong at exchange.microsoft.com>; Abhishek Sharma <ce.abhishek at samsung.com> Cc: iotivity-dev at lists.iotivity.org Subject: Re: [dev] Proposing oc_mutex_recursive While these reasons are all good when you?re designing / writing new code, recursive lock is better for what I am trying to do (see my email). If there is a strong objection, I can just implement it within ocstack itself and *not* share out publically. From: Dave Thaler Sent: Wednesday, April 5, 2017 10:19 AM To: Way Vadhanasin <Wayakorn.Vadhanasin at microsoft.com<mailto:Wayakorn.Vadhanasin at microsoft.com>>; jw0213.jung at samsung.com<mailto:jw0213.jung at samsung.com>; Soemin Tjong <stjong at exchange.microsoft.com<mailto:stjong at exchange.microsoft.com>>; Abhishek Sharma <ce.abhishek at samsung.com<mailto:ce.abhishek at samsung.com>> Cc: iotivity-dev at lists.iotivity.org<mailto:iotivity-dev at lists.iotivity.org>; Daniel Mihai <Daniel.Mihai at microsoft.com<mailto:Daniel.Mihai at microsoft.com>> Subject: RE: Proposing oc_mutex_recursive See the earlier ?proposal to enable PTHREAD_MUTEX_RECURSIVE? thread on the iotivity-dev list. Reasons to not have recursive locks are discussed on sites such as these: * http://zaval.org/resources/library/butenhof1.html<https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fzaval.org%2Fresources%2Flibrary%2Fbutenhof1.html&data=02%7C01%7CWayakorn.Vadhanasin%40microsoft.com%7C5b6c24049e594cddd34b08d47c49332d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636270101218850251&sdata=zL3ipfzEBS5Xeh0v%2Bz2jEEhQU38%2F8TiFM%2BiQHHFUkKE%3D&reserved=0> ? https://askldjd.com/2009/10/26/prefer-simple-mutex-over-recursive-mutex/<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Faskldjd.com%2F2009%2F10%2F26%2Fprefer-simple-mutex-over-recursive-mutex%2F&data=02%7C01%7CWayakorn.Vadhanasin%40microsoft.com%7C5b6c24049e594cddd34b08d47c49332d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636270101218860261&sdata=oxudD7Uh4xq5GgQI%2Fp%2FvPV7IOZb%2BWptAQfN5QhnjjHc%3D&reserved=0> * https://github.com/libuv/libuv/issues/1022<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Flibuv%2Flibuv%2Fissues%2F1022&data=02%7C01%7CWayakorn.Vadhanasin%40microsoft.com%7C5b6c24049e594cddd34b08d47c49332d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636270101218860261&sdata=nHJRTJq%2BeSNDBGqb8Mk8KSPxCNc0Pn3zv1JP6EjskQI%3D&reserved=0> is a discussion in another OSS project. * http://blog.stephencleary.com/2013/04/recursive-re-entrant-locks.html<https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.stephencleary.com%2F2013%2F04%2Frecursive-re-entrant-locks.html&data=02%7C01%7CWayakorn.Vadhanasin%40microsoft.com%7C5b6c24049e594cddd34b08d47c49332d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636270101218860261&sdata=hjiBETqAs%2BIkTSS%2Bx947typn9HrpN6wf3sw9AgQar0w%3D&reserved=0> From: Way Vadhanasin Sent: Wednesday, April 5, 2017 10:09 AM To: jw0213.jung at samsung.com<mailto:jw0213.jung at samsung.com>; Soemin Tjong <stjong at exchange.microsoft.com<mailto:stjong at exchange.microsoft.com>>; Abhishek Sharma <ce.abhishek at samsung.com<mailto:ce.abhishek at samsung.com>> Cc: iotivity-dev at lists.iotivity.org<mailto:iotivity-dev at lists.iotivity.org>; Daniel Mihai <Daniel.Mihai at microsoft.com<mailto:Daniel.Mihai at microsoft.com>>; Dave Thaler <dthaler at microsoft.com<mailto:dthaler at microsoft.com>> Subject: Proposing oc_mutex_recursive (renamed the title to catch attention regarding recursive lock) I?d like to propose adding a recursive lock in IoTivity C layer next to the existing oc_mutex implementation. The reason for this is to solve https://jira.iotivity.org/browse/IOT-1828<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fjira.iotivity.org%2Fbrowse%2FIOT-1828&data=02%7C01%7CWayakorn.Vadhanasin%40microsoft.com%7C5b6c24049e594cddd34b08d47c49332d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636270101218860261&sdata=GR2%2Bk2hFg5yWFMzwPeawY1e7Kj%2B5hp8uEP%2Bnfku%2B3Qw%3D&reserved=0> without introducing a complex lock and unlocking scheme to avoid recursion. In this work, I need to synchronize public APIs (in ocstack.h) as well as address the application callbacks. Without a recursive lock, I?d have to do what https://gerrit.iotivity.org/gerrit/#/c/18277/<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgerrit.iotivity.org%2Fgerrit%2F%23%2Fc%2F18277%2F&data=02%7C01%7CWayakorn.Vadhanasin%40microsoft.com%7C5b6c24049e594cddd34b08d47c49332d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636270101218860261&sdata=LnI9fMgNBvBbBKGv9eojgivMFY92aZbL59T5xgGlmsw%3D&reserved=0> is currently doing (please see the commit message), which seems to add unnecessary churn, complexity and more code to implement *Internal counterparts. Please let me know if anyone object me adding oc_mutex_recursive. Thanks, Way From: Way Vadhanasin Sent: Wednesday, March 29, 2017 1:14 AM To: 'jw0213.jung at samsung.com' <jw0213.jung at samsung.com<mailto:jw0213.jung at samsung.com>>; Soemin Tjong <stjong at exchange.microsoft.com<mailto:stjong at exchange.microsoft.com>>; Abhishek Sharma <ce.abhishek at samsung.com<mailto:ce.abhishek at samsung.com>> Cc: iotivity-dev at lists.iotivity.org<mailto:iotivity-dev at lists.iotivity.org> Subject: RE: [dev] Make OCProcess( ) needed only on single threaded platform Thanks for the feedbacks Jaewook. This means in addition to OCCancel, other public OC* APIs in ocstack.h also need to be serialized with this model. They got left out from my patch because all the samples and unit tests are running that in 2 independent phases so far (e.g., they call OCDoRequest for discovery, then they would run their OCProcess loop later). You?re correct that with the approach of running OCProcess in a worker thread owned by the stack, it needs to be prepared for applications at all time and thus guarantee that there are no data races. There?s also another approach of providing a new OCProcess-like API (let?s call it OCProcessWithShutdown), which blocks when there?s no work and returns after OCProcess tasks are complete, but this approach still requires the application to run it in a loop (but this model still requires a loop in the app). Here are our options: 1. Continue my patch below but add OCProcess serialization to other OC APIs. The advantage of this approach is no busy loop in the app. 2. Implement OCProcessWithShutdown. My preference is to remove the need for app to call the processing loop by pursuing #1. Please let me know your thoughts. Thanks, Way From: ??? [mailto:jw0213.j...@samsung.com] Sent: Wednesday, March 29, 2017 12:19 AM To: Soemin Tjong <stjong at exchange.microsoft.com<mailto:stjong at exchange.microsoft.com>>; Way Vadhanasin <Wayakorn.Vadhanasin at microsoft.com<mailto:Wayakorn.Vadhanasin at microsoft.com>>; Abhishek Sharma <ce.abhishek at samsung.com<mailto:ce.abhishek at samsung.com>> Cc: iotivity-dev at lists.iotivity.org<mailto:iotivity-dev at lists.iotivity.org> Subject: Re: [dev] Make OCProcess( ) needed only on single threaded platform Hi, Seomin Tjong. As Abhishek mentioned, we should consider global variables in multi-threaded environment. You still have a synchronization issue even though you make the thread run the 4 function "serially", since application have a chance to call csdk API that get into critical section. For example, if application call OCDoRequest() when the OCProcess thread is calling OCHandleResponse(), OCDoRequest() will try to add a client callback node to 'cbList' in occlientcb and OCHandleResponse() will try to delete a client callback node from 'cbList' at the same time, then it has a possibility of a segmentation fault. So before we make an extra thread run OCProcess() instead of application, we should handle all global variables in csdk(oc* files) first. This is not simple because the most of oc* modules that keep a head pointer of list as global variables give a node pointer to the other modules and they use it, so we can't just use a mutex lock for each global variables. I have been thinking about design for it. Let's talk about more on jira. Thanks, Jaewook Jung --------- Original Message --------- Sender : Soemin Tjong <stjong at exchange.microsoft.com<mailto:stjong at exchange.microsoft.com>> Date : 2017-03-29 03:37 (GMT+9) Title : Re: [dev] Make OCProcess( ) needed only on single threaded platform Forward to the list as Way?s comment didn?t get out to the list. Also, please see https://gerrit.iotivity.org/gerrit/18277<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgerrit.iotivity.org%2Fgerrit%2F18277&data=02%7C01%7Cstjong%40exchange.microsoft.com%7Cb729a6165fe54a43ebb108d47602c6af%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636263207847314741&sdata=flbbVCDFcXPTthfmhS%2F6nTu3h4tigUeBgUeZnNiA5GM%3D&reserved=0> for the change. From: Way Vadhanasin Sent: Thursday, March 23, 2017 12:07 AM To: Soemin Tjong <stjong at exchange.microsoft.com<mailto:stjong at exchange.microsoft.com>>; ce.abhishek at samsung.com<mailto:ce.abhishek at samsung.com> Cc: iotivity-dev at lists.iotivity.org<mailto:iotivity-dev at lists.iotivity.org> Subject: RE: [dev] Make OCProcess( ) needed only on single threaded platform Hi all, I looked into reasons that are causing IoTivity to wake up the CPU continuously and found 2 issues: 1. IOT-1828<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fjira.iotivity.org%2Fbrowse%2FIOT-1828&data=02%7C01%7CWayakorn.Vadhanasin%40microsoft.com%7C5b6c24049e594cddd34b08d47c49332d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636270101218860261&sdata=GR2%2Bk2hFg5yWFMzwPeawY1e7Kj%2B5hp8uEP%2Bnfku%2B3Qw%3D&reserved=0> ? applications calling OCProcess continuously in a loop. 2. IOT-1930<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fjira.iotivity.org%2Fbrowse%2FIOT-1930&data=02%7C01%7CWayakorn.Vadhanasin%40microsoft.com%7C5b6c24049e594cddd34b08d47c49332d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636270101218860261&sdata=iNOEMnHG28rN32xKxP1grZk%2FmBf1I42PnfVyyuKBSq4%3D&reserved=0> ? extlibs/timer is spinning endlessly (?loop()? thread start routine). For issue #1, I?m working on a fix to eliminate the need for applications to spin the OCProcess thread as discussed by Soemin below. The idea is for IoTivity stack to spin an extra thread to do the tasks that OCProcess is doing today, serially. And since the thread is owned by the stack, it will also manage when to run and when to idle the worker thread. Note that this work will not change the behavior of apps on Arduino since no worker threads can be spun on Arduino (this is guarded by SINGLE_THREAD define). So this work will only be in non-SINGLE_THREAD build. The existing OCProcess will be left alone for backward compatibility, but a deprecated warning (and @deprecated) will be shown if it is called from platforms other than Arduino. I?m also planning to update all the sample apps to take advantage of the new behavior. For apps that do not opt-in to the new model (i.e., those that still call OCProcess in a dedicated loop), they should only get a warning in the log. Functionalities will not be impacted. Please let me know if there are any concerns and feel free to send feedbacks my way. Thanks, Way From: Soemin Tjong Sent: Thursday, March 9, 2017 10:05 PM To: ce.abhishek at samsung.com<mailto:ce.abhishek at samsung.com> Cc: iotivity-dev at lists.iotivity.org<mailto:iotivity-dev at lists.iotivity.org>; Way Vadhanasin <Wayakorn.Vadhanasin at microsoft.com<mailto:Wayakorn.Vadhanasin at microsoft.com>> Subject: RE: [dev] Make OCProcess( ) needed only on single threaded platform Hi Abhishek, you are saying that there could be issues in running the 4 functions in multithreading scenario. That?s a good point, but perhaps the issues can be minimized by synchronizing the running of the 4 threads (at least for now), until issues are resolved. That way, we would still get the desire effects of not waking up unnecessarily. It?s not clear what you meant by csdk users not handling multi-thread scenario, can you describe more? I assume the users of the csdk apis are the (sample) apps, security, and C++ (OCPlatform, OCResource). And your point is that these apps could now get simultaneous calls from the 4 functions. If so then yes, it will need to be resolved as well after the stack side is fix. Or another possibility is to let the apps opt in. Thanks for the info Abhishek. From: Abhishek Sharma [mailto:ce.abhis...@samsung.com] Sent: Thursday, March 9, 2017 9:31 PM To: Soemin Tjong <stjong at exchange.microsoft.com<mailto:stjong at exchange.microsoft.com>> Cc: iotivity-dev at lists.iotivity.org<mailto:iotivity-dev at lists.iotivity.org>; Way Vadhanasin <Wayakorn.Vadhanasin at microsoft.com<mailto:Wayakorn.Vadhanasin at microsoft.com>> Subject: RE: [dev] Make OCProcess( ) needed only on single threaded platform Agree it would be great if app developers need not worry about popping that extra thread to keep calling OCProcess(). But I think removing this function now would be tricky and not limited to just removing single api. All users of csdk api's will require handling of multi-thread scenario, which they most probably might not be doing now. Also csdk (oc* files) will have to be reviewed to support multi-threading, for ex: global variables will have to be removed or put under lock. CA layer (ca* files) was designed to be multi-threaded from begining so you will face no issues there. Regards Abhishek Sharma --------- Original Message --------- Sender : Soemin Tjong <stjong at exchange.microsoft.com<mailto:stjong at exchange.microsoft.com>> Date : 2017-03-10 08:09 (GMT+5:30) Title : [dev] Make OCProcess( ) needed only on single threaded platform Hi all, we are investigating https://jira.iotivity.org/browse/IOT-1828<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fjira.iotivity.org%2Fbrowse%2FIOT-1828&data=02%7C01%7Cstjong%40exchange.microsoft.com%7C4b7f0cb9bddb4964bd8d08d46776a638%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636247206678818084&sdata=y%2BeydPdFTFUoy9BsBe5I%2BrIv%2F1nNMu4NkOkAMrJUEg8%3D&reserved=0> and can use some input. Currently code that uses ocstack needs to periodically call OCProcess( ) (most call it every 10 ms). The OCProcess( ) performs 4 functions: (1) process received packets, (2) handle presence, (3) handle gateway routing, and (4) handle TCP keepalive. Waking up often is not great for applications running on battery operated devices. We think calling OCProcess() should only be needed on single threaded platforms, like Arduino (i.e. when SINGLE_THREAD is defined). Looking at camessagehandler.c, it seems like the IoTivity code was at some point able to handle its own send & receive. Then SINGLE_HANDLE flag was set permanently and function (1) is now handled by periodic OCProcess( ) instead. Does anyone know the history of SINGLE_HANDLE? We want to propose making OCProcess( ) needed only on single threaded platform. To do that we?ll re-enable the receive thread, remove all references to SINGLE_HANDLE, and subsequently add new thread(s) to perform function 2 to 4. Can anyone think why this won?t work? Thanks! _______________________________________________ iotivity-dev mailing list iotivity-dev at lists.iotivity.org<mailto:iotivity-dev at lists.iotivity.org> https://lists.iotivity.org/mailman/listinfo/iotivity-dev<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.iotivity.org%2Fmailman%2Flistinfo%2Fiotivity-dev&data=02%7C01%7CWayakorn.Vadhanasin%40microsoft.com%7C5b6c24049e594cddd34b08d47c49332d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636270101218860261&sdata=gcUMJ5qUkGBE8bC2ZQ4UFM76Lvd2C0J3eFWve2PMvbU%3D&reserved=0> _______________________________________________ iotivity-dev mailing list iotivity-dev at lists.iotivity.org<mailto:iotivity-dev at lists.iotivity.org> https://lists.iotivity.org/mailman/listinfo/iotivity-dev<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.iotivity.org%2Fmailman%2Flistinfo%2Fiotivity-dev&data=02%7C01%7CWayakorn.Vadhanasin%40microsoft.com%7C5b6c24049e594cddd34b08d47c49332d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636270101218860261&sdata=gcUMJ5qUkGBE8bC2ZQ4UFM76Lvd2C0J3eFWve2PMvbU%3D&reserved=0> [cid:image001.gif at 01D2AF90.4E3233B0] [http://ext.samsung.net/mail/ext/v1/external/status/update?userid=jw0213.jung&do=bWFpbElEPTIwMTcwMzI5MDcxOTA2ZXBjbXMxcDUwZDFhMjg1MDgwZTAzMTBlMWJkZWIzZDIzZTlkNTE2MiZyZWNpcGllbnRBZGRyZXNzPVdheWFrb3JuLlZhZGhhbmFzaW5AbWljcm9zb2Z0LmNvbQ__] -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.iotivity.org/pipermail/iotivity-dev/attachments/20170407/fad961de/attachment.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: image001.gif Type: image/gif Size: 13402 bytes Desc: image001.gif URL: <http://lists.iotivity.org/pipermail/iotivity-dev/attachments/20170407/fad961de/attachment.gif>
