staging: lusture: lnet: Use list_for_each_entry

PATCH Superseded Bhaktipriya Shridhar 2 participants 8 messages Feb 22, 2016 – Feb 25, 2016 4 messages not in the archive Staging

lore

Share & tools

lore

Julia Lawall

2016-02-22 10:42 UTC #1 lore raw

Quoted 22 lines · Bhaktipriya Shridhar

In general, doubly linked lists are iterated using list_empty and list_entry functions. But it can be better written using list_for_each_entry macro.
This patch replaces the while loop containing list_empty and list_entry with list_for_each_entry and list_for_each_entry_safe(if list_del is used int the loop).
This was done with Coccinelle.
@@
expression E1;
identifier I1, I2;
type T;
iterator name list_for_each_entry;
@@
- while (list_empty(&E1) == 0)
+ list_for_each_entry (I1, &E1, I2)
{
	...when != T *I1;
(quoted diff) +0 -1
-	I1 = list_entry(E1.next, T, I2);

Quoted 2 lines

	...when != list_del(...);
	when != list_del_init(...);

No need to change here, but it is nicer to line up the when's.

Quoted 13 lines

}
@@
expression E1;
identifier I1, I2;
type T;
iterator name list_for_each_entry_safe;
@@
T *I1;
+ T *tmp;
...
- while (list_empty(&E1) == 0)
+ list_for_each_entry_safe (I1, tmp, &E1, I2)

Quoted 3 lines

{
	...when != T *I1;
	-	I1 = list_entry(E1.next, T, I2);

The - line is too indented.

Quoted 2 lines

	...
}

Good job on the semantic patch. Another possibility is converting list_for_each to list_for_each_entry. Someone did quite a few of these recently, but there may still be some opportunities. If you do find some opportunities, you may want to run make coccicheck on the result, because Coccinelle found a bunch of problems in the change that was done. I think that the issue is that with list_for_each_entry, the variable containing the entry is invalid after the normal end of the loop (ie without break), while when one implements the same thing by hand, the variable would naturally end up pointing to the last element.

julia

Quoted 35 lines

Signed-off-by Bhaktipriya Shridhar <[email protected]>
---
 drivers/staging/lustre/lnet/klnds/socklnd/socklnd_cb.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/staging/lustre/lnet/klnds/socklnd/socklnd_cb.c +2 -4
index efb7169..976fd78 100644
@@ -2354,13 +2354,12 @@ static inline void
 ksocknal_flush_stale_txs(ksock_peer_t *peer)
 {
 	ksock_tx_t *tx;
+	ksock_tx_t *tmp;
 	LIST_HEAD(stale_txs);
 
 	write_lock_bh(&ksocknal_data.ksnd_global_lock);
 
-	while (!list_empty(&peer->ksnp_tx_queue)) {
-		tx = list_entry(peer->ksnp_tx_queue.next, ksock_tx_t, tx_list);
-
+	list_for_each_entry_safe(tx, tmp, &peer->ksnp_tx_queue, tx_list) {
 		if (!cfs_time_aftereq(cfs_time_current(),
 				      tx->tx_deadline))
 			break;
--
2.1.4
--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20160222070345.GA10232%40Karyakshetra.
For more options, visit https://groups.google.com/d/optout.

Julia Lawall

2016-02-22 11:37 UTC #2 lore raw

Quoted 21 lines · Julia Lawall

Quoted 17 lines · Bhaktipriya Shridhar
In general, doubly linked lists are iterated using list_empty and list_entry functions. But it can be better written using list_for_each_entry macro.
This patch replaces the while loop containing list_empty and list_entry with list_for_each_entry and list_for_each_entry_safe(if list_del is used int the loop).
This was done with Coccinelle.
@@
expression E1;
identifier I1, I2;
type T;
iterator name list_for_each_entry;
@@

Quoted 7 lines

Quoted 7 lines
- while (list_empty(&E1) == 0)
+ list_for_each_entry (I1, &E1, I2)
{
	...when != T *I1;
(quoted diff) +0 -1
-	I1 = list_entry(E1.next, T, I2);
	...when != list_del(...);
	when != list_del_init(...);

Actually, I think that this rule makes no sense. If there is no delete in the loop, the list will never become empty. Empty doesn't mean being at the end of the list. It means that there is really nothing in the list.

julia

Quoted 73 lines

No need to change here, but it is nicer to line up the when's.
Quoted 13 lines
}
@@
expression E1;
identifier I1, I2;
type T;
iterator name list_for_each_entry_safe;
@@
T *I1;
+ T *tmp;
...
- while (list_empty(&E1) == 0)
+ list_for_each_entry_safe (I1, tmp, &E1, I2)
Quoted 3 lines
{
	...when != T *I1;
	-	I1 = list_entry(E1.next, T, I2);
The - line is too indented.
Quoted 2 lines
	...
}
Good job on the semantic patch. Another possibility is converting list_for_each to list_for_each_entry. Someone did quite a few of these recently, but there may still be some opportunities. If you do find some opportunities, you may want to run make coccicheck on the result, because Coccinelle found a bunch of problems in the change that was done. I think that the issue is that with list_for_each_entry, the variable containing the entry is invalid after the normal end of the loop (ie without break), while when one implements the same thing by hand, the variable would naturally end up pointing to the last element.
julia
Quoted 35 lines
Signed-off-by Bhaktipriya Shridhar <[email protected]>
---
 drivers/staging/lustre/lnet/klnds/socklnd/socklnd_cb.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/staging/lustre/lnet/klnds/socklnd/socklnd_cb.c +2 -4
index efb7169..976fd78 100644
@@ -2354,13 +2354,12 @@ static inline void
 ksocknal_flush_stale_txs(ksock_peer_t *peer)
 {
 	ksock_tx_t *tx;
+	ksock_tx_t *tmp;
 	LIST_HEAD(stale_txs);
 
 	write_lock_bh(&ksocknal_data.ksnd_global_lock);
 
-	while (!list_empty(&peer->ksnp_tx_queue)) {
-		tx = list_entry(peer->ksnp_tx_queue.next, ksock_tx_t, tx_list);
-
+	list_for_each_entry_safe(tx, tmp, &peer->ksnp_tx_queue, tx_list) {
 		if (!cfs_time_aftereq(cfs_time_current(),
 				      tx->tx_deadline))
 			break;
--
2.1.4
--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20160222070345.GA10232%40Karyakshetra.
For more options, visit https://groups.google.com/d/optout.

Julia Lawall

2016-02-22 11:38 UTC #3 lore raw

In reply to Julia Lawall

Quoted 6 lines

Quoted 6 lines
@@
expression E1;
identifier I1, I2;
type T;
iterator name list_for_each_entry_safe;
@@

Quoted 8 lines

Quoted 8 lines
T *I1;
+ T *tmp;
...
- while (list_empty(&E1) == 0)
+ list_for_each_entry_safe (I1, tmp, &E1, I2)
{
	...when != T *I1;
	-	I1 = list_entry(E1.next, T, I2);

Could you try this rule on the entire kernel? If there are many occurrences, then maybe it would be a good idea to include this rule with the other semantic patches in the kernel.

julia

Bhaktipriya Shridhar

author

2016-02-22 13:14 UTC #4 lore raw

Hello Julia,

Thank you for your reply. I ran the rule that you mentioned on the entire kernel.
I found that around 70 files will be needing this change.

Thanks,
Bhaktipriya

Quoted 21 lines · Julia Lawall

Quoted 15 lines
Quoted 15 lines
@@
expression E1;
identifier I1, I2;
type T;
iterator name list_for_each_entry_safe;
@@
T *I1;
+ T *tmp;
...
- while (list_empty(&E1) == 0)
+ list_for_each_entry_safe (I1, tmp, &E1, I2)
{
...when != T *I1;
- I1 = list_entry(E1.next, T, I2);
Could you try this rule on the entire kernel? If there are many occurrences, then maybe it would be a good idea to include this rule with the other semantic patches in the kernel.
julia

Julia Lawall

2016-02-22 13:24 UTC #5 lore raw

Quoted 5 lines · Bhaktipriya Shridhar

Hello Julia,
Thank you for your reply. I ran the rule that you mentioned on the entire kernel.
I found that around 70 files will be needing this change.

Cool. Do the changes look correct as well?

I'll get back to you on how you can make it suitable for incusion in the Linux kernel.

julia

Quoted 34 lines

Thanks,
Bhaktipriya
Quoted 16 lines · Julia Lawall
Quoted 15 lines
Quoted 15 lines
@@
expression E1;
identifier I1, I2;
type T;
iterator name list_for_each_entry_safe;
@@
T *I1;
+ T *tmp;
...
- while (list_empty(&E1) == 0)
+ list_for_each_entry_safe (I1, tmp, &E1, I2)
{
...when != T *I1;
- I1 = list_entry(E1.next, T, I2);
Quoted 5 lines
Could you try this rule on the entire kernel? If there are many occurrences, then maybe it would be a good idea to include this rule with the other semantic patches in the kernel.
julia
--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/CAFNHFT85ZZkLBR8Tr9rMgvynHRdnzugvUFGiACydvEqKYD721g%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

Bhaktipriya Shridhar

author

2016-02-22 13:50 UTC #6 lore raw

Hello Julia,

Quoted 4 lines · Julia Lawall

On Mon, 22 Feb 2016, Bhaktipriya Shridhar wrote:

Quoted 8 lines

Quoted 5 lines
Hello Julia,
Thank you for your reply. I ran the rule that you mentioned on the entire kernel.
I found that around 70 files will be needing this change.
Cool. Do the changes look correct as well?

Yes . The changes look fine.

Quoted 3 lines

I'll get back to you on how you can make it suitable for incusion in the Linux kernel.

Sure. Looking forward to hearing from you soon.

Thanks,
Bhaktipriya

A message that is not in the archive (sent off-list or not yet received): alpine.DEB.2.10.1602220908480.2532@hadrien

A message that is not in the archive (sent off-list or not yet received): CAFNHFT_OyT+ZrjmYkByJwtJP+xLH5r_hXV_QfYZat6-B1p0zLA@mail.gmail.com

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): CAFNHFT9j_KQjfGw1SVUxGp=Bh1fYX2Jb1HnFgi3qXMzfuzx1qg@mail.gmail.com

Bhaktipriya Shridhar

author

2016-02-25 11:10 UTC #11 lore raw

Hello Julia,

Can you please review the attached semantic patch. This addresses the list_for_each_entry_safe issue.
I have prepared the semantic patch as per your instructions and have also tested it with the -D patch option.

Thanks,
Bhaktipriya

Quoted 110 lines · Bhaktipriya Shridhar

Hello Julia,
I have prepared the semantic patch as per your instructions. I also tested it using the -D patch flag. It is working fine at my end. I have attached the file for your review. Please have a look.
Thanks,
Bhaktipriya
Quoted 98 lines · Julia Lawall
Quoted 6 lines · Bhaktipriya Shridhar
Hello Julia,
I was running spgen on my patch and wanted to know how I could answer the following questions:
1.Specify options for the script or press <enter> to continue:
Probably --include-headers --no-includes is fine. This means that your semantic patch doesn't need any information from header files, such as type information. But header files will be treated individually, in case the issue occurs in the header file also.
Quoted 1 line
2.Rule "r". Write a message for org mode:
Just put something concise, like you would put in a patch subject line.
Quoted 2 lines
3.Rule "r". Write a message for report or press <enter> to reuse the org message.
Just press enter.
julia
Quoted 73 lines
Can you please guide me.
Thanks,
Bhaktipriya
Quoted 66 lines · Julia Lawall
Quoted 5 lines · Bhaktipriya Shridhar
Hello Julia,
Thank you for your reply. I ran the rule that you mentioned on the entire kernel.
I found that around 70 files will be needing this change.
OK, you can get the latest version of Coccinelle from github. If you make and install that, you will get a tool called spgen. Run that on your semantic patch, and answer the various questions. It can help to have an example of a semantic patch that is already in the kernel in front of you to have an idea of how to answer the questions.
The resulting semantic patch is printed on standard output. You can also give a -o argument to get it put in a file, in the usual way.
You can test the result with the argument -D patch, to get a patch, -D context, to get the relevant lines highlighted, -D org --no-show-diff to get org mode (I can check this if you don't know what it is), and -D report --no-show-diff to get some printed report about where issues arise. The size of the output for patch and context should be about the same, but patch would be bigger, because it also has + lines. The size of report and org should also be about the same size. Or maybe the size of report and the number of lines containing TODO in the org mode result.
When you are done, you can just send the result to me. I won't be very available during the coming day though.
julia
Quoted 34 lines
Thanks,
Bhaktipriya
Quoted 16 lines · Julia Lawall
Quoted 15 lines
Quoted 15 lines
@@
expression E1;
identifier I1, I2;
type T;
iterator name list_for_each_entry_safe;
@@
T *I1;
+ T *tmp;
...
- while (list_empty(&E1) == 0)
+ list_for_each_entry_safe (I1, tmp, &E1, I2)
{
...when != T *I1;
- I1 = list_entry(E1.next, T, I2);
Quoted 5 lines
Could you try this rule on the entire kernel? If there are many occurrences, then maybe it would be a good idea to include this rule with the other semantic patches in the kernel.
julia
--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/CAFNHFT85ZZkLBR8Tr9rMgvynHRdnzugvUFGiACydvEqKYD721g%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.