| Message ID | 20240722150141.31391-5-jgross@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mini-os: cleanup of mm.c | expand |
Juergen Gross, le lun. 22 juil. 2024 17:01:41 +0200, a ecrit: > Remove the sanity_check() function, as it is used nowhere. > > Since any application linked with Mini-OS can't call sanity_check() > either (there is no EXPORT_SYMBOL for it), there is zero chance of > breaking any use case. Don't we still want to keep it around, at least as formal documentation of the expected status of the list? Samuel > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > include/lib.h | 3 --- > mm.c | 16 ---------------- > 2 files changed, 19 deletions(-) > > diff --git a/include/lib.h b/include/lib.h > index abd4e9ab..acd4acc6 100644 > --- a/include/lib.h > +++ b/include/lib.h > @@ -152,9 +152,6 @@ do { \ > > #define BUG_ON(x) ASSERT(!(x)) > > -/* Consistency check as much as possible. */ > -void sanity_check(void); > - > /* Get own domid. */ > domid_t get_domid(void); > > diff --git a/mm.c b/mm.c > index 96686a5c..1fa7e7bf 100644 > --- a/mm.c > +++ b/mm.c > @@ -394,19 +394,3 @@ void init_mm(void) > void fini_mm(void) > { > } > - > -void sanity_check(void) > -{ > - int x; > - chunk_head_t *head; > - > - for ( x = 0; x < FREELIST_SIZE; x++ ) > - { > - for ( head = free_list[x].next; !FREELIST_EMPTY(head); > - head = head->next ) > - { > - ASSERT(!allocated_in_map(virt_to_pfn(head))); > - ASSERT(head->next->prev == head); > - } > - } > -} > -- > 2.43.0 >
On 22.07.24 23:35, Samuel Thibault wrote: > Juergen Gross, le lun. 22 juil. 2024 17:01:41 +0200, a ecrit: >> Remove the sanity_check() function, as it is used nowhere. >> >> Since any application linked with Mini-OS can't call sanity_check() >> either (there is no EXPORT_SYMBOL for it), there is zero chance of >> breaking any use case. > > Don't we still want to keep it around, at least as formal documentation > of the expected status of the list? Hmm, is it really worth the extra code? There are 2 ASSERT()s I'm deleting: one testing the allocation bitmap to match the comment further up: /* * ALLOCATION BITMAP * One bit per page of memory. Bit set => page is allocated. */ And the other one testing the linked lists being correct, which IMO doesn't need any further documentation. Juergen
Hello, Jürgen Groß, le mar. 23 juil. 2024 08:36:13 +0200, a ecrit: > On 22.07.24 23:35, Samuel Thibault wrote: > > Juergen Gross, le lun. 22 juil. 2024 17:01:41 +0200, a ecrit: > > > Remove the sanity_check() function, as it is used nowhere. > > > > > > Since any application linked with Mini-OS can't call sanity_check() > > > either (there is no EXPORT_SYMBOL for it), there is zero chance of > > > breaking any use case. > > > > Don't we still want to keep it around, at least as formal documentation > > of the expected status of the list? > > Hmm, is it really worth the extra code? I have already seen such kind of piece of code getting very convenient when tracking odd bugs. Samuel
On 25.07.24 00:44, Samuel Thibault wrote: > Hello, > > Jürgen Groß, le mar. 23 juil. 2024 08:36:13 +0200, a ecrit: >> On 22.07.24 23:35, Samuel Thibault wrote: >>> Juergen Gross, le lun. 22 juil. 2024 17:01:41 +0200, a ecrit: >>>> Remove the sanity_check() function, as it is used nowhere. >>>> >>>> Since any application linked with Mini-OS can't call sanity_check() >>>> either (there is no EXPORT_SYMBOL for it), there is zero chance of >>>> breaking any use case. >>> >>> Don't we still want to keep it around, at least as formal documentation >>> of the expected status of the list? >> >> Hmm, is it really worth the extra code? > > I have already seen such kind of piece of code getting very convenient > when tracking odd bugs. What about putting it under CONFIG_TEST then? This would keep it in the source and it would even be compile tested by a simple "make testbuild", which is a simple test I'm always doing before submitting a patch. Juergen
Jürgen Groß, le jeu. 25 juil. 2024 08:25:18 +0200, a ecrit: > On 25.07.24 00:44, Samuel Thibault wrote: > > Hello, > > > > Jürgen Groß, le mar. 23 juil. 2024 08:36:13 +0200, a ecrit: > > > On 22.07.24 23:35, Samuel Thibault wrote: > > > > Juergen Gross, le lun. 22 juil. 2024 17:01:41 +0200, a ecrit: > > > > > Remove the sanity_check() function, as it is used nowhere. > > > > > > > > > > Since any application linked with Mini-OS can't call sanity_check() > > > > > either (there is no EXPORT_SYMBOL for it), there is zero chance of > > > > > breaking any use case. > > > > > > > > Don't we still want to keep it around, at least as formal documentation > > > > of the expected status of the list? > > > > > > Hmm, is it really worth the extra code? > > > > I have already seen such kind of piece of code getting very convenient > > when tracking odd bugs. > > What about putting it under CONFIG_TEST then? Ok ! Samuel
On 25.07.24 08:29, Samuel Thibault wrote: > Jürgen Groß, le jeu. 25 juil. 2024 08:25:18 +0200, a ecrit: >> On 25.07.24 00:44, Samuel Thibault wrote: >>> Hello, >>> >>> Jürgen Groß, le mar. 23 juil. 2024 08:36:13 +0200, a ecrit: >>>> On 22.07.24 23:35, Samuel Thibault wrote: >>>>> Juergen Gross, le lun. 22 juil. 2024 17:01:41 +0200, a ecrit: >>>>>> Remove the sanity_check() function, as it is used nowhere. >>>>>> >>>>>> Since any application linked with Mini-OS can't call sanity_check() >>>>>> either (there is no EXPORT_SYMBOL for it), there is zero chance of >>>>>> breaking any use case. >>>>> >>>>> Don't we still want to keep it around, at least as formal documentation >>>>> of the expected status of the list? >>>> >>>> Hmm, is it really worth the extra code? >>> >>> I have already seen such kind of piece of code getting very convenient >>> when tracking odd bugs. >> >> What about putting it under CONFIG_TEST then? > > Ok ! I went a little bit further by calling sanity_check() from periodic_thread() in test.c once a second. This will make real use of the function. Juergen
diff --git a/include/lib.h b/include/lib.h index abd4e9ab..acd4acc6 100644 --- a/include/lib.h +++ b/include/lib.h @@ -152,9 +152,6 @@ do { \ #define BUG_ON(x) ASSERT(!(x)) -/* Consistency check as much as possible. */ -void sanity_check(void); - /* Get own domid. */ domid_t get_domid(void); diff --git a/mm.c b/mm.c index 96686a5c..1fa7e7bf 100644 --- a/mm.c +++ b/mm.c @@ -394,19 +394,3 @@ void init_mm(void) void fini_mm(void) { } - -void sanity_check(void) -{ - int x; - chunk_head_t *head; - - for ( x = 0; x < FREELIST_SIZE; x++ ) - { - for ( head = free_list[x].next; !FREELIST_EMPTY(head); - head = head->next ) - { - ASSERT(!allocated_in_map(virt_to_pfn(head))); - ASSERT(head->next->prev == head); - } - } -}
Remove the sanity_check() function, as it is used nowhere. Since any application linked with Mini-OS can't call sanity_check() either (there is no EXPORT_SYMBOL for it), there is zero chance of breaking any use case. Signed-off-by: Juergen Gross <jgross@suse.com> --- include/lib.h | 3 --- mm.c | 16 ---------------- 2 files changed, 19 deletions(-)