| Message ID | 20161117202523.GA35194@macpro.local (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
On Thu, Nov 17, 2016 at 12:25 PM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > > Would something like the following be fine? This looks good to me, although I didn't actually test it. But it looks like what I would have expected. Some of those inlines look large enough that I wonder how much sense they make as inlines any more, but I think that's a separate issue. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 17, 2016 at 02:03:05PM -0800, Linus Torvalds wrote: > On Thu, Nov 17, 2016 at 12:25 PM, Luc Van Oostenryck > <luc.vanoostenryck@gmail.com> wrote: > > > > Would something like the following be fine? > > This looks good to me, although I didn't actually test it. But it > looks like what I would have expected. > > Some of those inlines look large enough that I wonder how much sense > they make as inlines any more, but I think that's a separate issue. Oh, I absolutely agree. What I would like is something more iterator oriented which keep track of the head-list-nr state. I have a working prototype I made last week but it still needs some more polishing. Luc -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 17, 2016 at 09:25:24PM +0100, Luc Van Oostenryck wrote: > Would something like the following be fine? > ... > I've quickly checked it on the testsuite (and it seems to pass ;). > I'll validate this more thoroughly but I won't be able to do this > just now. OK, I've run it on a much bigger set: a make C=2 kernel with allyesconfig, the same tree as the test I did previously and it gives me the same result and, of course, no crashes. So for me the changes are OK. Tested-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 18, 2016 at 01:29:19AM +0100, Luc Van Oostenryck wrote: > On Thu, Nov 17, 2016 at 02:03:05PM -0800, Linus Torvalds wrote: > > On Thu, Nov 17, 2016 at 12:25 PM, Luc Van Oostenryck > > <luc.vanoostenryck@gmail.com> wrote: > > > > > > Would something like the following be fine? > > > > This looks good to me, although I didn't actually test it. But it > > looks like what I would have expected. > > > > Some of those inlines look large enough that I wonder how much sense > > they make as inlines any more, but I think that's a separate issue. > > Oh, I absolutely agree. > What I would like is something more iterator oriented which keep track > of the head-list-nr state. I have a working prototype I made last week > but it still needs some more polishing. > I've worked a bit more on this. I had a nice, clean & compact implementation where basically all ptr_list walking was done by something like: struct ptr_iter iter; ptr_iter_init(&iter, head); while ((ptr = ptr_iter_next(&iter))) ... Of course, still hidden under the macros which do the type checking. This had the advantage that all the logic was in a single place. It seemed to work well that is until I discovered that at a few places we're storing null pointers in ptr_lists. It's thus not possible to use the returned pointer to also check the end of list condition. The situation is even a bit more complex because the PREPARE/NEXT_PTR_LIST() also can't work with null pointers. I've found a few alternative that work in all the cases but they aren't simple & compact enough to my taste so I'll let things like they are and maybe just sent a few cleanups later. Luc -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 28, 2016 at 1:15 PM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > > I've worked a bit more on this. I had a nice, clean & compact > implementation where basically all ptr_list walking was done by > something like: > struct ptr_iter iter; > ptr_iter_init(&iter, head); > while ((ptr = ptr_iter_next(&iter))) > ... Last time I try that, I look at the machine code and realized that using iter struct is not quite the same as using macro. On each function boundary, the compiler still try to sync the data register content into the iter structure in memory. In other words, there is no way to make the iter variable as register, it always get sync into memory. I did run a full kernel source check back then, did not find an observable difference. The time difference is within the variance of each run. Maybe due to most of the list are very short. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/ptrlist.h b/ptrlist.h index 61e159fd..d09be2f5 100644 --- a/ptrlist.h +++ b/ptrlist.h @@ -67,28 +67,51 @@ extern int linearize_ptr_list(struct ptr_list *, void **, int); static inline void *first_ptr_list(struct ptr_list *list) { + struct ptr_list *head = list; + if (!list) return NULL; + + while (list->nr == 0) { + list = list->next; + if (list == head) + return NULL; + } return PTR_ENTRY(list, 0); } static inline void *last_ptr_list(struct ptr_list *list) { + struct ptr_list *head = list; if (!list) return NULL; list = list->prev; + while (list->nr == 0) { + if (list == head) + return NULL; + list = list->prev; + } return PTR_ENTRY(list, list->nr-1); } +#define PTR_DEREF(__head, idx, PTR_ENTRY) ({ \ + struct ptr_list *__list = __head; \ + while (__list && __list->nr == 0) { \ + __list = __list->next; \ + if (__list == __head) \ + __list = NULL; \ + } \ + __list ? PTR_ENTRY(__list, idx) : NULL; \ +}) + #define DO_PREPARE(head, ptr, __head, __list, __nr, PTR_ENTRY) \ do { \ struct ptr_list *__head = (struct ptr_list *) (head); \ struct ptr_list *__list = __head; \ int __nr = 0; \ CHECK_TYPE(head,ptr); \ - if (__head) ptr = PTR_ENTRY(__head, 0); \ - else ptr = NULL + ptr = PTR_DEREF(__head, 0, PTR_ENTRY); \ #define DO_NEXT(ptr, __head, __list, __nr, PTR_ENTRY) \ if (ptr) { \ @@ -110,7 +133,7 @@ static inline void *last_ptr_list(struct ptr_list *list) do { \ __nr = 0; \ __list = __head; \ - if (__head) ptr = PTR_ENTRY(__head, 0); \ + if (__head) ptr = PTR_DEREF(__head, 0, PTR_ENTRY); \ } while (0) #define DO_FINISH(ptr, __head, __list, __nr) \