* [Cluster-devel] [PATCH 3/6] gfs2: simplify current_tail() via list_last_entry_or_null() @ 2013-11-15 4:32 Jeff Liu 2013-11-15 16:39 ` Oleg Nesterov 0 siblings, 1 reply; 4+ messages in thread From: Jeff Liu @ 2013-11-15 4:32 UTC (permalink / raw) To: cluster-devel.redhat.com From: Jie Liu <jeff.liu@oracle.com> Simplify the code in current_tail() via list_last_entry_or_null(). Signed-off-by: Jie Liu <jeff.liu@oracle.com> --- fs/gfs2/log.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c index 610613f..555f767 100644 --- a/fs/gfs2/log.c +++ b/fs/gfs2/log.c @@ -441,13 +441,9 @@ static unsigned int current_tail(struct gfs2_sbd *sdp) spin_lock(&sdp->sd_ail_lock); - if (list_empty(&sdp->sd_ail1_list)) { - tail = sdp->sd_log_head; - } else { - tr = list_entry(sdp->sd_ail1_list.prev, struct gfs2_trans, - tr_list); - tail = tr->tr_first; - } + tr = list_last_entry_or_null(&sdp->sd_ail1_list, struct gfs2_trans, + tr_list); + tail = tr ? tr->tr_first : sdp->sd_log_head; spin_unlock(&sdp->sd_ail_lock); -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [Cluster-devel] [PATCH 3/6] gfs2: simplify current_tail() via list_last_entry_or_null() 2013-11-15 4:32 [Cluster-devel] [PATCH 3/6] gfs2: simplify current_tail() via list_last_entry_or_null() Jeff Liu @ 2013-11-15 16:39 ` Oleg Nesterov 2013-11-15 17:12 ` [Cluster-devel] introduce list_get_first/last (Was: [PATCH 3/6] gfs2: simplify current_tail() via list_last_entry_or_null()) Oleg Nesterov 0 siblings, 1 reply; 4+ messages in thread From: Oleg Nesterov @ 2013-11-15 16:39 UTC (permalink / raw) To: cluster-devel.redhat.com On 11/15, Jeff Liu wrote: > > @@ -441,13 +441,9 @@ static unsigned int current_tail(struct gfs2_sbd *sdp) > > spin_lock(&sdp->sd_ail_lock); > > - if (list_empty(&sdp->sd_ail1_list)) { > - tail = sdp->sd_log_head; > - } else { > - tr = list_entry(sdp->sd_ail1_list.prev, struct gfs2_trans, > - tr_list); > - tail = tr->tr_first; > - } > + tr = list_last_entry_or_null(&sdp->sd_ail1_list, struct gfs2_trans, > + tr_list); > + tail = tr ? tr->tr_first : sdp->sd_log_head; > Personally I agree with Steven. At least in this case list_last_entry_or_null() doesn't really help to simplify the code. But probably list_last_entry() makes sense in the "else" branch, athough this is minor. Off-topic. Not sure this really makes sense, but I was thinking about list_get_first(pos, head, member) \ ((pos) = list_first_entry(head, typeof(*pos), member)) and list_get_first() last of course. The obvious advantage is that compared to tr = list_last_entry(sdp->sd_ail1_list, struct gfs2_trans, tr_list); above you do not need to type "struct gfs2_trans", list_get_last(tr, sdp->sd_ail1_list, tr_list); looks a bit better. Oleg. ^ permalink raw reply [flat|nested] 4+ messages in thread
* [Cluster-devel] introduce list_get_first/last (Was: [PATCH 3/6] gfs2: simplify current_tail() via list_last_entry_or_null()) 2013-11-15 16:39 ` Oleg Nesterov @ 2013-11-15 17:12 ` Oleg Nesterov 2013-11-18 5:41 ` Jeff Liu 0 siblings, 1 reply; 4+ messages in thread From: Oleg Nesterov @ 2013-11-15 17:12 UTC (permalink / raw) To: cluster-devel.redhat.com On 11/15, Oleg Nesterov wrote: > > But probably list_last_entry() makes sense in the "else" branch, > athough this is minor. > > > Off-topic. Not sure this really makes sense, but I was thinking about > > list_get_first(pos, head, member) \ > ((pos) = list_first_entry(head, typeof(*pos), member)) > > and list_get_first() last of course. The obvious advantage is that > compared to > > tr = list_last_entry(sdp->sd_ail1_list, struct gfs2_trans, tr_list); > > above you do not need to type "struct gfs2_trans", > > list_get_last(tr, sdp->sd_ail1_list, tr_list); > > looks a bit better. And list.h can use it too. So how about the patch below? I can't decide whether it cleanups or uglfies the code, I do not trust my taste ;) But to me it looks a bit better this way. Oleg. -- diff --git a/include/linux/list.h b/include/linux/list.h index ef95941..f52aba8 100644 --- a/include/linux/list.h +++ b/include/linux/list.h @@ -372,6 +372,12 @@ static inline void list_splice_tail_init(struct list_head *list, #define list_last_entry(ptr, type, member) \ list_entry((ptr)->prev, type, member) +#define list_get_first(pos, head, member) \ + ((pos) = list_first_entry(head, typeof(*(pos)), member)) + +#define list_get_last(pos, head, member) \ + ((pos) = list_last_entry(head, typeof(*(pos)), member)) + /** * list_first_entry_or_null - get the first element from a list * @ptr: the list head to take the element from. @@ -443,7 +449,7 @@ static inline void list_splice_tail_init(struct list_head *list, * @member: the name of the list_struct within the struct. */ #define list_for_each_entry(pos, head, member) \ - for (pos = list_first_entry(head, typeof(*pos), member); \ + for (list_get_first(pos, head, member); \ &pos->member != (head); \ pos = list_next_entry(pos, member)) @@ -454,7 +460,7 @@ static inline void list_splice_tail_init(struct list_head *list, * @member: the name of the list_struct within the struct. */ #define list_for_each_entry_reverse(pos, head, member) \ - for (pos = list_last_entry(head, typeof(*pos), member); \ + for (list_get_last(pos, head, member); \ &pos->member != (head); \ pos = list_prev_entry(pos, member)) @@ -517,7 +523,7 @@ static inline void list_splice_tail_init(struct list_head *list, * @member: the name of the list_struct within the struct. */ #define list_for_each_entry_safe(pos, n, head, member) \ - for (pos = list_first_entry(head, typeof(*pos), member), \ + for (list_get_first(pos, head, member), \ n = list_next_entry(pos, member); \ &pos->member != (head); \ pos = n, n = list_next_entry(n, member)) @@ -564,7 +570,7 @@ static inline void list_splice_tail_init(struct list_head *list, * of list entry. */ #define list_for_each_entry_safe_reverse(pos, n, head, member) \ - for (pos = list_last_entry(head, typeof(*pos), member), \ + for (list_get_last(pos, head, member), \ n = list_prev_entry(pos, member); \ &pos->member != (head); \ pos = n, n = list_prev_entry(n, member)) ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [Cluster-devel] introduce list_get_first/last (Was: [PATCH 3/6] gfs2: simplify current_tail() via list_last_entry_or_null()) 2013-11-15 17:12 ` [Cluster-devel] introduce list_get_first/last (Was: [PATCH 3/6] gfs2: simplify current_tail() via list_last_entry_or_null()) Oleg Nesterov @ 2013-11-18 5:41 ` Jeff Liu 0 siblings, 0 replies; 4+ messages in thread From: Jeff Liu @ 2013-11-18 5:41 UTC (permalink / raw) To: cluster-devel.redhat.com Hi Oleg, Sorry for my late response! On 11/16/2013 01:12 AM, Oleg Nesterov wrote: > On 11/15, Oleg Nesterov wrote: >> >> But probably list_last_entry() makes sense in the "else" branch, >> athough this is minor. >> >> >> Off-topic. Not sure this really makes sense, but I was thinking about >> >> list_get_first(pos, head, member) \ >> ((pos) = list_first_entry(head, typeof(*pos), member)) >> >> and list_get_first() last of course. The obvious advantage is that >> compared to >> >> tr = list_last_entry(sdp->sd_ail1_list, struct gfs2_trans, tr_list); >> >> above you do not need to type "struct gfs2_trans", >> >> list_get_last(tr, sdp->sd_ail1_list, tr_list); >> >> looks a bit better. > > And list.h can use it too. > > So how about the patch below? I can't decide whether it cleanups or > uglfies the code, I do not trust my taste ;) But to me it looks a > bit better this way. I personally think this is better for GFS2 case if I understood correctly, and it can make the existing list helper a bit neat per my taste :) The current list_first_entry() works fine consider a common pattern below: while (!list_empty(ptr)) { struct dummy *p = list_first_entry(ptr, struct dummy, member); .... } But I'm not sure if we can find out more external use cases can get benefits with list_get_first/last... Thanks, -Jeff > > Oleg. > -- > > diff --git a/include/linux/list.h b/include/linux/list.h > index ef95941..f52aba8 100644 > --- a/include/linux/list.h > +++ b/include/linux/list.h > @@ -372,6 +372,12 @@ static inline void list_splice_tail_init(struct list_head *list, > #define list_last_entry(ptr, type, member) \ > list_entry((ptr)->prev, type, member) > > +#define list_get_first(pos, head, member) \ > + ((pos) = list_first_entry(head, typeof(*(pos)), member)) > + > +#define list_get_last(pos, head, member) \ > + ((pos) = list_last_entry(head, typeof(*(pos)), member)) > + > /** > * list_first_entry_or_null - get the first element from a list > * @ptr: the list head to take the element from. > @@ -443,7 +449,7 @@ static inline void list_splice_tail_init(struct list_head *list, > * @member: the name of the list_struct within the struct. > */ > #define list_for_each_entry(pos, head, member) \ > - for (pos = list_first_entry(head, typeof(*pos), member); \ > + for (list_get_first(pos, head, member); \ > &pos->member != (head); \ > pos = list_next_entry(pos, member)) > > @@ -454,7 +460,7 @@ static inline void list_splice_tail_init(struct list_head *list, > * @member: the name of the list_struct within the struct. > */ > #define list_for_each_entry_reverse(pos, head, member) \ > - for (pos = list_last_entry(head, typeof(*pos), member); \ > + for (list_get_last(pos, head, member); \ > &pos->member != (head); \ > pos = list_prev_entry(pos, member)) > > @@ -517,7 +523,7 @@ static inline void list_splice_tail_init(struct list_head *list, > * @member: the name of the list_struct within the struct. > */ > #define list_for_each_entry_safe(pos, n, head, member) \ > - for (pos = list_first_entry(head, typeof(*pos), member), \ > + for (list_get_first(pos, head, member), \ > n = list_next_entry(pos, member); \ > &pos->member != (head); \ > pos = n, n = list_next_entry(n, member)) > @@ -564,7 +570,7 @@ static inline void list_splice_tail_init(struct list_head *list, > * of list entry. > */ > #define list_for_each_entry_safe_reverse(pos, n, head, member) \ > - for (pos = list_last_entry(head, typeof(*pos), member), \ > + for (list_get_last(pos, head, member), \ > n = list_prev_entry(pos, member); \ > &pos->member != (head); \ > pos = n, n = list_prev_entry(n, member)) > > _______________________________________________ > xfs mailing list > xfs at oss.sgi.com > http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-11-18 5:41 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-11-15 4:32 [Cluster-devel] [PATCH 3/6] gfs2: simplify current_tail() via list_last_entry_or_null() Jeff Liu 2013-11-15 16:39 ` Oleg Nesterov 2013-11-15 17:12 ` [Cluster-devel] introduce list_get_first/last (Was: [PATCH 3/6] gfs2: simplify current_tail() via list_last_entry_or_null()) Oleg Nesterov 2013-11-18 5:41 ` Jeff Liu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).