* [PATCH] ipc: use list_for_each_entry for list traversing @ 2013-04-05 13:42 Nikola Pajkovsky 2013-04-08 22:38 ` Andrew Morton 0 siblings, 1 reply; 4+ messages in thread From: Nikola Pajkovsky @ 2013-04-05 13:42 UTC (permalink / raw) To: Stanislav Kinsbursky Cc: Andrew Morton, Eric W. Biederman, Peter Hurley, linux-kernel the ipc/msg.c code does all list operations by hand and it open-codes the accesses, instead of using for_each_entry. Signed-off-by: Nikola Pajkovsky <npajkovs@redhat.com> --- ipc/msg.c | 35 ++++++++--------------------------- 1 files changed, 8 insertions(+), 27 deletions(-) diff --git a/ipc/msg.c b/ipc/msg.c index fede1d0..8eca57a 100644 --- a/ipc/msg.c +++ b/ipc/msg.c @@ -237,14 +237,9 @@ static inline void ss_del(struct msg_sender *mss) static void ss_wakeup(struct list_head *h, int kill) { - struct list_head *tmp; + struct msg_sender *mss, *t; - tmp = h->next; - while (tmp != h) { - struct msg_sender *mss; - - mss = list_entry(tmp, struct msg_sender, list); - tmp = tmp->next; + list_for_each_entry_safe(mss, t, h, list) { if (kill) mss->list.next = NULL; wake_up_process(mss->tsk); @@ -253,14 +248,9 @@ static void ss_wakeup(struct list_head *h, int kill) static void expunge_all(struct msg_queue *msq, int res) { - struct list_head *tmp; - - tmp = msq->q_receivers.next; - while (tmp != &msq->q_receivers) { - struct msg_receiver *msr; + struct msg_receiver *msr, *t; - msr = list_entry(tmp, struct msg_receiver, r_list); - tmp = tmp->next; + list_for_each_entry_safe(msr, t, &msq->q_receivers, r_list) { msr->r_msg = NULL; wake_up_process(msr->r_tsk); smp_mb(); @@ -278,7 +268,7 @@ static void expunge_all(struct msg_queue *msq, int res) */ static void freeque(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp) { - struct list_head *tmp; + struct msg_msg *msg; struct msg_queue *msq = container_of(ipcp, struct msg_queue, q_perm); expunge_all(msq, -EIDRM); @@ -286,11 +276,7 @@ static void freeque(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp) msg_rmid(ns, msq); msg_unlock(msq); - tmp = msq->q_messages.next; - while (tmp != &msq->q_messages) { - struct msg_msg *msg = list_entry(tmp, struct msg_msg, m_list); - - tmp = tmp->next; + list_for_each_entry(msg, &msq->q_messages, m_list) { atomic_dec(&ns->msg_hdrs); free_msg(msg); } @@ -602,14 +588,9 @@ static int testmsg(struct msg_msg *msg, long type, int mode) static inline int pipelined_send(struct msg_queue *msq, struct msg_msg *msg) { - struct list_head *tmp; - - tmp = msq->q_receivers.next; - while (tmp != &msq->q_receivers) { - struct msg_receiver *msr; + struct msg_receiver *msr, *t; - msr = list_entry(tmp, struct msg_receiver, r_list); - tmp = tmp->next; + list_for_each_entry_safe(msr, t, &msq->q_receivers, r_list) { if (testmsg(msg, msr->r_msgtype, msr->r_mode) && !security_msg_queue_msgrcv(msq, msg, msr->r_tsk, msr->r_msgtype, msr->r_mode)) { -- 1.7.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ipc: use list_for_each_entry for list traversing 2013-04-05 13:42 [PATCH] ipc: use list_for_each_entry for list traversing Nikola Pajkovsky @ 2013-04-08 22:38 ` Andrew Morton 2013-04-09 9:39 ` Nikola Pajkovsky 0 siblings, 1 reply; 4+ messages in thread From: Andrew Morton @ 2013-04-08 22:38 UTC (permalink / raw) To: Nikola Pajkovsky Cc: Stanislav Kinsbursky, Eric W. Biederman, Peter Hurley, linux-kernel On Fri, 5 Apr 2013 15:42:11 +0200 Nikola Pajkovsky <npajkovs@redhat.com> wrote: > the ipc/msg.c code does all list operations by hand and it open-codes > the accesses, instead of using for_each_entry. > > ... > > --- a/ipc/msg.c > +++ b/ipc/msg.c > @@ -237,14 +237,9 @@ static inline void ss_del(struct msg_sender *mss) > > static void ss_wakeup(struct list_head *h, int kill) > { > - struct list_head *tmp; > + struct msg_sender *mss, *t; > > - tmp = h->next; > - while (tmp != h) { > - struct msg_sender *mss; > - > - mss = list_entry(tmp, struct msg_sender, list); > - tmp = tmp->next; > + list_for_each_entry_safe(mss, t, h, list) { > if (kill) > mss->list.next = NULL; > wake_up_process(mss->tsk); urgh, that code is sick. What's it doing poking around in the list_head internals? > @@ -253,14 +248,9 @@ static void ss_wakeup(struct list_head *h, int kill) > > static void expunge_all(struct msg_queue *msq, int res) > { > - struct list_head *tmp; > - > - tmp = msq->q_receivers.next; > - while (tmp != &msq->q_receivers) { > - struct msg_receiver *msr; > + struct msg_receiver *msr, *t; > > - msr = list_entry(tmp, struct msg_receiver, r_list); > - tmp = tmp->next; > + list_for_each_entry_safe(msr, t, &msq->q_receivers, r_list) { > msr->r_msg = NULL; > wake_up_process(msr->r_tsk); > smp_mb(); I think list_for_each_entry() would suffice here. > @@ -278,7 +268,7 @@ static void expunge_all(struct msg_queue *msq, int res) > */ > static void freeque(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp) > { > - struct list_head *tmp; > + struct msg_msg *msg; > struct msg_queue *msq = container_of(ipcp, struct msg_queue, q_perm); > > expunge_all(msq, -EIDRM); > @@ -286,11 +276,7 @@ static void freeque(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp) > msg_rmid(ns, msq); > msg_unlock(msq); > > - tmp = msq->q_messages.next; > - while (tmp != &msq->q_messages) { > - struct msg_msg *msg = list_entry(tmp, struct msg_msg, m_list); > - > - tmp = tmp->next; > + list_for_each_entry(msg, &msq->q_messages, m_list) { > atomic_dec(&ns->msg_hdrs); > free_msg(msg); > } This is buggy isn't it? list_for_each_entry() will use the recently-freed `msg'. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ipc: use list_for_each_entry for list traversing 2013-04-08 22:38 ` Andrew Morton @ 2013-04-09 9:39 ` Nikola Pajkovsky 2013-04-09 20:36 ` Andrew Morton 0 siblings, 1 reply; 4+ messages in thread From: Nikola Pajkovsky @ 2013-04-09 9:39 UTC (permalink / raw) To: Andrew Morton Cc: Stanislav Kinsbursky, Eric W. Biederman, Peter Hurley, linux-kernel Andrew Morton <akpm@linux-foundation.org> writes: > On Fri, 5 Apr 2013 15:42:11 +0200 Nikola Pajkovsky <npajkovs@redhat.com> wrote: > >> the ipc/msg.c code does all list operations by hand and it open-codes >> the accesses, instead of using for_each_entry. >> >> ... >> >> --- a/ipc/msg.c >> +++ b/ipc/msg.c >> @@ -237,14 +237,9 @@ static inline void ss_del(struct msg_sender *mss) >> >> static void ss_wakeup(struct list_head *h, int kill) >> { >> - struct list_head *tmp; >> + struct msg_sender *mss, *t; >> >> - tmp = h->next; >> - while (tmp != h) { >> - struct msg_sender *mss; >> - >> - mss = list_entry(tmp, struct msg_sender, list); >> - tmp = tmp->next; >> + list_for_each_entry_safe(mss, t, h, list) { >> if (kill) >> mss->list.next = NULL; >> wake_up_process(mss->tsk); > > urgh, that code is sick. What's it doing poking around in the > list_head internals? No idea, it there from beginning of first kernel importation into git. Where is history before git? >> @@ -253,14 +248,9 @@ static void ss_wakeup(struct list_head *h, int kill) >> >> static void expunge_all(struct msg_queue *msq, int res) >> { >> - struct list_head *tmp; >> - >> - tmp = msq->q_receivers.next; >> - while (tmp != &msq->q_receivers) { >> - struct msg_receiver *msr; >> + struct msg_receiver *msr, *t; >> >> - msr = list_entry(tmp, struct msg_receiver, r_list); >> - tmp = tmp->next; >> + list_for_each_entry_safe(msr, t, &msq->q_receivers, r_list) { >> msr->r_msg = NULL; >> wake_up_process(msr->r_tsk); >> smp_mb(); > > I think list_for_each_entry() would suffice here. I don't know, I found wake_up_sem_queue_do in sem.c and it looks almost same except preempt stuff. I'll be dancing around ipc/ >> @@ -278,7 +268,7 @@ static void expunge_all(struct msg_queue *msq, int res) >> */ >> static void freeque(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp) >> { >> - struct list_head *tmp; >> + struct msg_msg *msg; >> struct msg_queue *msq = container_of(ipcp, struct msg_queue, q_perm); >> >> expunge_all(msq, -EIDRM); >> @@ -286,11 +276,7 @@ static void freeque(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp) >> msg_rmid(ns, msq); >> msg_unlock(msq); >> >> - tmp = msq->q_messages.next; >> - while (tmp != &msq->q_messages) { >> - struct msg_msg *msg = list_entry(tmp, struct msg_msg, m_list); >> - >> - tmp = tmp->next; >> + list_for_each_entry(msg, &msq->q_messages, m_list) { >> atomic_dec(&ns->msg_hdrs); >> free_msg(msg); >> } > > This is buggy isn't it? list_for_each_entry() will use the > recently-freed `msg'. yes, it is. --8<-----8<-----8<-----8<-----8<-----8<--- From: Nikola Pajkovsky <npajkovs@redhat.com> Date: Thu, 4 Apr 2013 19:23:55 +0200 Subject: [PATCH v2] ipc: use list_for_each_entry_[safe] for list traversing the ipc/msg.c code does all list operations by hand and it open-codes the accesses, instead of using for_each_entry_[safe]. v2: in freeque there has to be used safe version of list_for_each_entry Signed-off-by: Nikola Pajkovsky <npajkovs@redhat.com> --- ipc/msg.c | 35 ++++++++--------------------------- 1 files changed, 8 insertions(+), 27 deletions(-) diff --git a/ipc/msg.c b/ipc/msg.c index fede1d0..f45ef14 100644 --- a/ipc/msg.c +++ b/ipc/msg.c @@ -237,14 +237,9 @@ static inline void ss_del(struct msg_sender *mss) static void ss_wakeup(struct list_head *h, int kill) { - struct list_head *tmp; + struct msg_sender *mss, *t; - tmp = h->next; - while (tmp != h) { - struct msg_sender *mss; - - mss = list_entry(tmp, struct msg_sender, list); - tmp = tmp->next; + list_for_each_entry_safe(mss, t, h, list) { if (kill) mss->list.next = NULL; wake_up_process(mss->tsk); @@ -253,14 +248,9 @@ static void ss_wakeup(struct list_head *h, int kill) static void expunge_all(struct msg_queue *msq, int res) { - struct list_head *tmp; - - tmp = msq->q_receivers.next; - while (tmp != &msq->q_receivers) { - struct msg_receiver *msr; + struct msg_receiver *msr, *t; - msr = list_entry(tmp, struct msg_receiver, r_list); - tmp = tmp->next; + list_for_each_entry_safe(msr, t, &msq->q_receivers, r_list) { msr->r_msg = NULL; wake_up_process(msr->r_tsk); smp_mb(); @@ -278,7 +268,7 @@ static void expunge_all(struct msg_queue *msq, int res) */ static void freeque(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp) { - struct list_head *tmp; + struct msg_msg *msg, *t; struct msg_queue *msq = container_of(ipcp, struct msg_queue, q_perm); expunge_all(msq, -EIDRM); @@ -286,11 +276,7 @@ static void freeque(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp) msg_rmid(ns, msq); msg_unlock(msq); - tmp = msq->q_messages.next; - while (tmp != &msq->q_messages) { - struct msg_msg *msg = list_entry(tmp, struct msg_msg, m_list); - - tmp = tmp->next; + list_for_each_entry_safe(msg, t, &msq->q_messages, m_list) { atomic_dec(&ns->msg_hdrs); free_msg(msg); } @@ -602,14 +588,9 @@ static int testmsg(struct msg_msg *msg, long type, int mode) static inline int pipelined_send(struct msg_queue *msq, struct msg_msg *msg) { - struct list_head *tmp; - - tmp = msq->q_receivers.next; - while (tmp != &msq->q_receivers) { - struct msg_receiver *msr; + struct msg_receiver *msr, *t; - msr = list_entry(tmp, struct msg_receiver, r_list); - tmp = tmp->next; + list_for_each_entry_safe(msr, t, &msq->q_receivers, r_list) { if (testmsg(msg, msr->r_msgtype, msr->r_mode) && !security_msg_queue_msgrcv(msq, msg, msr->r_tsk, msr->r_msgtype, msr->r_mode)) { -- 1.7.1 -- Nikola ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ipc: use list_for_each_entry for list traversing 2013-04-09 9:39 ` Nikola Pajkovsky @ 2013-04-09 20:36 ` Andrew Morton 0 siblings, 0 replies; 4+ messages in thread From: Andrew Morton @ 2013-04-09 20:36 UTC (permalink / raw) To: Nikola Pajkovsky Cc: Stanislav Kinsbursky, Eric W. Biederman, Peter Hurley, linux-kernel On Tue, 09 Apr 2013 11:39:07 +0200 Nikola Pajkovsky <npajkovs@redhat.com> wrote: > Andrew Morton <akpm@linux-foundation.org> writes: > > > urgh, that code is sick. What's it doing poking around in the > > list_head internals? > > No idea, it there from beginning of first kernel importation into > git. Where is history before git? The old bitkeeper repo is at git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/old-2.6-bkcvs.git and goes back to the 2.4->2.5 split iirc. But this code predates that - my trusty CVS tree goes back to linux-2.4.2-pre2 (November 2000) and the list.next hackery is in there. > >> @@ -253,14 +248,9 @@ static void ss_wakeup(struct list_head *h, int kill) > >> > >> static void expunge_all(struct msg_queue *msq, int res) > >> { > >> - struct list_head *tmp; > >> - > >> - tmp = msq->q_receivers.next; > >> - while (tmp != &msq->q_receivers) { > >> - struct msg_receiver *msr; > >> + struct msg_receiver *msr, *t; > >> > >> - msr = list_entry(tmp, struct msg_receiver, r_list); > >> - tmp = tmp->next; > >> + list_for_each_entry_safe(msr, t, &msq->q_receivers, r_list) { > >> msr->r_msg = NULL; > >> wake_up_process(msr->r_tsk); > >> smp_mb(); > > > > I think list_for_each_entry() would suffice here. > > I don't know, I found wake_up_sem_queue_do in sem.c and it looks almost same > except preempt stuff. I'll be dancing around ipc/ wake_up_sem_queue_do() is wrong ;) ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-04-09 20:36 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-04-05 13:42 [PATCH] ipc: use list_for_each_entry for list traversing Nikola Pajkovsky 2013-04-08 22:38 ` Andrew Morton 2013-04-09 9:39 ` Nikola Pajkovsky 2013-04-09 20:36 ` Andrew Morton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox