Messages in this thread | | | Date | Thu, 22 May 2008 16:16:25 -0500 | From | Michael Halcrow <> | Subject | Re: [PATCH] eCryptfs: Clean up kthread synchronization |
| |
On Thu, May 22, 2008 at 12:41:42PM -0700, Andrew Morton wrote: > On Thu, 22 May 2008 14:31:55 -0500 > Michael Halcrow <mhalcrow@us.ibm.com> wrote: > > > > > +void ecryptfs_destroy_kthread(void) > > > > +{ > > > > + struct ecryptfs_open_req tmp_req; > > > > + struct ecryptfs_open_req *req; > > > > + > > > > + mutex_lock(&ecryptfs_kthread_ctl.mux); > > > > + ecryptfs_kthread_ctl.flags |= ECRYPTFS_KTHREAD_ZOMBIE; > > > > + list_for_each_entry(req, &ecryptfs_kthread_ctl.req_list, > > > > + kthread_ctl_list) { > > > > + mutex_lock(&req->mux); > > > > + req->flags |= ECRYPTFS_REQ_ZOMBIE; > > > > + wake_up_process(req->requesting_task); > > > > + mutex_unlock(&req->mux); > > > > + } > > > > + memset(&tmp_req, 0, sizeof(tmp_req)); > > > > + tmp_req.flags = ECRYPTFS_REQ_ZOMBIE; > > > > + list_add_tail(&tmp_req.kthread_ctl_list, > > > > + &ecryptfs_kthread_ctl.req_list); > > > > + mutex_unlock(&ecryptfs_kthread_ctl.mux); > > > > + wake_up(&ecryptfs_kthread_ctl.wait); > > > > +} > > > > > > eh? We attach a local variable to a global list and then return? > > > That won't last very long. > > > > Adding this dummy entry to the list is just my own way of getting the > > kthread to wake up and shut down. This actually works, albeit a little > > ugly. The list and its contents get dropped on the floor at this point > > because (ecryptfs_kthread_ctl.flags & ECRYPTFS_KTHREAD_ZOMBIE). The > > only consumer of this list (the kthread) checks for this flag > > immediately after getting the mux, and if it is there, it just > > exits. The only producer on this list (ecryptfs_privileged_open()) > > checks for this flag immediately after getting the mux and bows out if > > it is set. In other words, once this flag is set, the list and its > > contents become untouchable by anything other than > > ecryptfs_destroy_kthread(). > > Unconvinced. > > As soon as ecryptfs_destroy_kthread() returns, tmp_req is destroyed. > But it remains on ecryptfs_kthread_ctl.req_list.
I intend for ecryptfs_kthread_ctl.req_list to be irrelevant once ecryptfs_destroy_kthread() grabs the mux and sets (ecryptfs_kthread_ctl.flags |= ECRYPTFS_KTHREAD_ZOMBIE); nobody will ever do anything with that list any more. The state of the list -- including the dangling list pointer -- simply does not matter any more.
> > memset(&tmp_req, 0, sizeof(tmp_req)); > > tmp_req.flags = ECRYPTFS_REQ_ZOMBIE; > > list_add_tail(&tmp_req.kthread_ctl_list, > > &ecryptfs_kthread_ctl.req_list); > > mutex_unlock(&ecryptfs_kthread_ctl.mux); > > wake_up(&ecryptfs_kthread_ctl.wait); > > -> it's dead.
This is what gets woken up:
--- wait_event_freezable( ecryptfs_kthread_ctl.wait, !list_empty(&ecryptfs_kthread_ctl.req_list)); mutex_lock(&ecryptfs_kthread_ctl.mux); if (ecryptfs_kthread_ctl.flags & ECRYPTFS_KTHREAD_ZOMBIE) { mutex_unlock(&ecryptfs_kthread_ctl.mux); goto out; } ---
So the flag causes the kthread to just quit, ignoring the list altogether.
| |