bufcache KNF

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

bufcache KNF

Martin Pieuchot
ok?

Index: vfs_bio.c
===================================================================
RCS file: /cvs/src/sys/kern/vfs_bio.c,v
retrieving revision 1.173
diff -u -p -r1.173 vfs_bio.c
--- vfs_bio.c 10 Mar 2016 03:09:45 -0000 1.173
+++ vfs_bio.c 10 Mar 2016 07:15:57 -0000
@@ -1292,14 +1292,14 @@ buf_adjcnt(struct buf *bp, long ncount)
  * this function is called when a hot or warm queue may have exceeded its
  * size limit. it will move a buf to the coldqueue.
  */
-int chillbufs(struct
-    bufcache *cache, struct bufqueue *queue, int64_t *queuepages);
+int chillbufs(struct bufcache *, struct bufqueue *, int64_t *);
 
 void
 bufcache_init(void)
 {
  int i;
- for (i=0; i < NUM_CACHES; i++) {
+
+ for (i = 0; i < NUM_CACHES; i++) {
  TAILQ_INIT(&cleancache[i].hotqueue);
  TAILQ_INIT(&cleancache[i].coldqueue);
  TAILQ_INIT(&cleancache[i].warmqueue);
@@ -1314,7 +1314,8 @@ void
 bufcache_adjust(void)
 {
  int i;
- for (i=0; i < NUM_CACHES; i++) {
+
+ for (i = 0; i < NUM_CACHES; i++) {
  while (chillbufs(&cleancache[i], &cleancache[i].warmqueue,
     &cleancache[i].warmbufpages) ||
     chillbufs(&cleancache[i], &cleancache[i].hotqueue,
@@ -1393,7 +1394,7 @@ bufcache_getcleanbuf_range(int start, in
 struct buf *
 bufcache_getanycleanbuf(void)
 {
- return bufcache_getcleanbuf_range(DMA_CACHE, NUM_CACHES -1, 0);
+ return bufcache_getcleanbuf_range(DMA_CACHE, NUM_CACHES-1, 0);
 }
 
 
@@ -1407,15 +1408,15 @@ void
 bufcache_take(struct buf *bp)
 {
  struct bufqueue *queue;
- int64_t pages;
+ struct bufcache *cache = &cleancache[bp->cache];
+ int64_t pages = atop(bp->b_bufsize);
 
  splassert(IPL_BIO);
 
  KASSERT(ISSET(bp->b_flags, B_BC));
  KASSERT(bp->cache >= DMA_CACHE);
  KASSERT((bp->cache < NUM_CACHES));
- pages = atop(bp->b_bufsize);
- struct bufcache *cache = &cleancache[bp->cache];
+
  if (!ISSET(bp->b_flags, B_DELWRI)) {
                 if (ISSET(bp->b_flags, B_COLD)) {
  queue = &cache->coldqueue;
@@ -1467,14 +1468,16 @@ void
 bufcache_release(struct buf *bp)
 {
  struct bufqueue *queue;
- int64_t pages;
  struct bufcache *cache = &cleancache[bp->cache];
- pages = atop(bp->b_bufsize);
+ int64_t pages = atop(bp->b_bufsize);
+
  KASSERT(ISSET(bp->b_flags, B_BC));
  KASSERT((ISSET(bp->b_flags, B_DMA) && bp->cache == 0)
     || ((!ISSET(bp->b_flags, B_DMA)) && bp->cache > 0));
+
  if (!ISSET(bp->b_flags, B_DELWRI)) {
  int64_t *queuepages;
+
  if (ISSET(bp->b_flags, B_WARM | B_COLD)) {
  SET(bp->b_flags, B_WARM);
  CLR(bp->b_flags, B_COLD);
@@ -1510,7 +1513,7 @@ hibernate_suspend_bufcache(void)
 
  s = splbio();
  /* Chuck away all the cache pages.. discard bufs, do not promote */
- while ((bp = bufcache_getcleanbuf_range(DMA_CACHE, NUM_CACHES - 1, 1))) {
+ while ((bp = bufcache_getcleanbuf_range(DMA_CACHE, NUM_CACHES-1, 1))) {
  bufcache_take(bp);
  if (bp->b_vp) {
  RB_REMOVE(buf_rb_bufs,

Reply | Threaded
Open this post in threaded view
|

Re: bufcache KNF

Bob Beck-2
no. youre giving me random conflicts. unless you have a reason beyond
turdshining now is not good time to do that

On Thursday, 10 March 2016, Martin Pieuchot <[hidden email]> wrote:

> ok?
>
> Index: vfs_bio.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/vfs_bio.c,v
> retrieving revision 1.173
> diff -u -p -r1.173 vfs_bio.c
> --- vfs_bio.c   10 Mar 2016 03:09:45 -0000      1.173
> +++ vfs_bio.c   10 Mar 2016 07:15:57 -0000
> @@ -1292,14 +1292,14 @@ buf_adjcnt(struct buf *bp, long ncount)
>   * this function is called when a hot or warm queue may have exceeded its
>   * size limit. it will move a buf to the coldqueue.
>   */
> -int chillbufs(struct
> -    bufcache *cache, struct bufqueue *queue, int64_t *queuepages);
> +int chillbufs(struct bufcache *, struct bufqueue *, int64_t *);
>
>  void
>  bufcache_init(void)
>  {
>         int i;
> -       for (i=0; i < NUM_CACHES; i++) {
> +
> +       for (i = 0; i < NUM_CACHES; i++) {
>                 TAILQ_INIT(&cleancache[i].hotqueue);
>                 TAILQ_INIT(&cleancache[i].coldqueue);
>                 TAILQ_INIT(&cleancache[i].warmqueue);
> @@ -1314,7 +1314,8 @@ void
>  bufcache_adjust(void)
>  {
>         int i;
> -       for (i=0; i < NUM_CACHES; i++) {
> +
> +       for (i = 0; i < NUM_CACHES; i++) {
>                 while (chillbufs(&cleancache[i], &cleancache[i].warmqueue,
>                     &cleancache[i].warmbufpages) ||
>                     chillbufs(&cleancache[i], &cleancache[i].hotqueue,
> @@ -1393,7 +1394,7 @@ bufcache_getcleanbuf_range(int start, in
>  struct buf *
>  bufcache_getanycleanbuf(void)
>  {
> -       return bufcache_getcleanbuf_range(DMA_CACHE, NUM_CACHES -1, 0);
> +       return bufcache_getcleanbuf_range(DMA_CACHE, NUM_CACHES-1, 0);
>  }
>
>
> @@ -1407,15 +1408,15 @@ void
>  bufcache_take(struct buf *bp)
>  {
>         struct bufqueue *queue;
> -       int64_t pages;
> +       struct bufcache *cache = &cleancache[bp->cache];
> +       int64_t pages = atop(bp->b_bufsize);
>
>         splassert(IPL_BIO);
>
>         KASSERT(ISSET(bp->b_flags, B_BC));
>         KASSERT(bp->cache >= DMA_CACHE);
>         KASSERT((bp->cache < NUM_CACHES));
> -       pages = atop(bp->b_bufsize);
> -       struct bufcache *cache = &cleancache[bp->cache];
> +
>         if (!ISSET(bp->b_flags, B_DELWRI)) {
>                  if (ISSET(bp->b_flags, B_COLD)) {
>                         queue = &cache->coldqueue;
> @@ -1467,14 +1468,16 @@ void
>  bufcache_release(struct buf *bp)
>  {
>         struct bufqueue *queue;
> -       int64_t pages;
>         struct bufcache *cache = &cleancache[bp->cache];
> -       pages = atop(bp->b_bufsize);
> +       int64_t pages = atop(bp->b_bufsize);
> +
>         KASSERT(ISSET(bp->b_flags, B_BC));
>         KASSERT((ISSET(bp->b_flags, B_DMA) && bp->cache == 0)
>             || ((!ISSET(bp->b_flags, B_DMA)) && bp->cache > 0));
> +
>         if (!ISSET(bp->b_flags, B_DELWRI)) {
>                 int64_t *queuepages;
> +
>                 if (ISSET(bp->b_flags, B_WARM | B_COLD)) {
>                         SET(bp->b_flags, B_WARM);
>                         CLR(bp->b_flags, B_COLD);
> @@ -1510,7 +1513,7 @@ hibernate_suspend_bufcache(void)
>
>         s = splbio();
>         /* Chuck away all the cache pages.. discard bufs, do not promote */
> -       while ((bp = bufcache_getcleanbuf_range(DMA_CACHE, NUM_CACHES - 1,
> 1))) {
> +       while ((bp = bufcache_getcleanbuf_range(DMA_CACHE, NUM_CACHES-1,
> 1))) {
>                 bufcache_take(bp);
>                 if (bp->b_vp) {
>                         RB_REMOVE(buf_rb_bufs,
>
>
Reply | Threaded
Open this post in threaded view
|

Re: bufcache KNF

Ted Unangst-6
In reply to this post by Martin Pieuchot
Martin Pieuchot wrote:
> ok?
>
> -int chillbufs(struct
> -    bufcache *cache, struct bufqueue *queue, int64_t *queuepages);
> +int chillbufs(struct bufcache *, struct bufqueue *, int64_t *);

fwiw i like names in prototypes, so i know what's going on. i know style says
that, but i think the advice is obsolete.

Reply | Threaded
Open this post in threaded view
|

Re: bufcache KNF

Mark Kettenis
> From: "Ted Unangst" <[hidden email]>
> Date: Mon, 11 Apr 2016 09:44:26 -0400
>
> Martin Pieuchot wrote:
> > ok?
> >
> > -int chillbufs(struct
> > -    bufcache *cache, struct bufqueue *queue, int64_t *queuepages);
> > +int chillbufs(struct bufcache *, struct bufqueue *, int64_t *);
>
> fwiw i like names in prototypes, so i know what's going on. i know style says
> that, but i think the advice is obsolete.

At the same time I don't like prototypes that span multiple lines; you
can't win.

And prototypes with names in public headers are still an issue.

Reply | Threaded
Open this post in threaded view
|

Re: bufcache KNF

Mike Belopuhov-5
On 11 April 2016 at 15:51, Mark Kettenis <[hidden email]> wrote:
>
> And prototypes with names in public headers are still an issue.
>

Interesting point.  What's a public header though?
Are files that end up in /usr/include/dev/pci/ public headers?
If so, why do we install all of them indiscriminately?

Reply | Threaded
Open this post in threaded view
|

Re: bufcache KNF

Ted Unangst-6
In reply to this post by Mark Kettenis
Mark Kettenis wrote:
> And prototypes with names in public headers are still an issue.

I think you misspelled standard. :)

Reply | Threaded
Open this post in threaded view
|

Re: bufcache KNF

Bob Beck-2
guys. i have stuff outstanding in here. find something else to bikeshed
please

On Monday, 11 April 2016, Ted Unangst <[hidden email]> wrote:

> Mark Kettenis wrote:
> > And prototypes with names in public headers are still an issue.
>
> I think you misspelled standard. :)
>
>
Reply | Threaded
Open this post in threaded view
|

Re: bufcache KNF

Michael McConville-3
In reply to this post by Ted Unangst-6
Ted Unangst wrote:
> Martin Pieuchot wrote:
> > ok?
> >
> > -int chillbufs(struct
> > -    bufcache *cache, struct bufqueue *queue, int64_t *queuepages);
> > +int chillbufs(struct bufcache *, struct bufqueue *, int64_t *);
>
> fwiw i like names in prototypes, so i know what's going on. i know
> style says that, but i think the advice is obsolete.

The compiler doesn't check that the argument names in the prototype
match those in the definition. The below program compiles without
warning.


int f(int a);

int
f(int b)
{
        return b+3;
}

int
main()
{
        return f(2);
}

Reply | Threaded
Open this post in threaded view
|

Re: bufcache KNF

Miod Vallat

>> fwiw i like names in prototypes, so i know what's going on. i know
>> style says that, but i think the advice is obsolete.
>
> The compiler doesn't check that the argument names in the prototype
> match those in the definition. The below program compiles without
> warning.

This is not the point. The point is that putting argument names in
public headers increases the risk of breaking third-party software
thanks to the preprocessor.

Reply | Threaded
Open this post in threaded view
|

Re: bufcache KNF

Michael McConville-3
Miod Vallat wrote:

>
> >> fwiw i like names in prototypes, so i know what's going on. i know
> >> style says that, but i think the advice is obsolete.
> >
> > The compiler doesn't check that the argument names in the prototype
> > match those in the definition. The below program compiles without
> > warning.
>
> This is not the point. The point is that putting argument names in
> public headers increases the risk of breaking third-party software
> thanks to the preprocessor.

I wasn't suggesting that that's the only reason. I was just pointing it
out in case someone suggested the guideline "only specify argument names
in file-local prototypes".

Reply | Threaded
Open this post in threaded view
|

Re: bufcache KNF

Philip Guenther-2
In reply to this post by Miod Vallat
On Mon, Apr 11, 2016 at 11:49 AM, Miod Vallat <[hidden email]> wrote:
> The point is that putting argument names in
> public headers increases the risk of breaking third-party software
> thanks to the preprocessor.

The safe way for the implementation (us!) to do that is to use
identifiers that start with an underbar and a lowercase letter, ala:

int   ptrace(int _request, pid_t _pid, caddr_t _addr, int _data);

as that namespace is reserved for implementations; applications may
not define macros that match those.


Philip Guenther

Reply | Threaded
Open this post in threaded view
|

Re: bufcache KNF

lists-2
In reply to this post by Mike Belopuhov-5
Mon, 11 Apr 2016 15:59:53 +0200 Mike Belopuhov <[hidden email]>
> On 11 April 2016 at 15:51, Mark Kettenis <[hidden email]> wrote:
> >
> > And prototypes with names in public headers are still an issue.
> >  
>
> Interesting point.  What's a public header though?
> Are files that end up in /usr/include/dev/pci/ public headers?
> If so, why do we install all of them indiscriminately?

With the risk of quoting the obvious, public here means reusable.

$ mandoc /usr/share/man/man9/style.9 | grep -B1 -A5 -ni prototyp \
> | sed '/83/q'
56-
57:     All functions are prototyped somewhere.
58-
59:     Function prototypes for private functions (i.e., functions not used
60-     elsewhere) go at the top of the first source module.  In userland,
61-     functions local to one source module should be declared ‘static’.  This
62-     should not be done in kernel land since it makes it impossible to use the
63-     kernel debugger.
64-
65:     Functions used from other parts of the kernel are prototyped in the
66-     relevant include file.
67-
68-     Functions that are used locally in more than one module go into a
69-     separate header file, e.g., extern.h.
70-
71:     Prototypes should not have variable names associated with the types;
72-     i.e.,
73-
74-           void    function(int);
75-     not:
76-           void    function(int a);
77-
78:     Prototypes may have an extra space after a tab to enable function names
79-     to line up:
80-
81-           static char     *function(int, const char *);
82-           static void      usage(void);
83-