sparc64 softraid boot: workaround for memory leak

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

sparc64 softraid boot: workaround for memory leak

Stefan Sperling-5
On my T5220 LDOM guests cannot boot from softraid because ofwboot
crashes with a "Fast Data Access MMU Miss" while loading the kernel:

>> OpenBSD BOOT 1.9                                      
ERROR: /iscsi-hba: No iscsi-network-bootpath property                      
sr0*                                            
Passphrase:                          
Booting sr0:a/bsd          
8480888@0x1000000
ERROR: Last Trap: Fast Data Access MMU Miss

I've tracked down the failure to a crash in OF_open() called in sr_strategy().
There is no missing OF_close() call. So it seems a memory/resource leak
of some kind is happening in firmware during OF_open()/OF_close().

Affected firmware version info:

        SP firmware 3.0.12.8.a
        SP firmware build number: 108523
        SP firmware date: Fri Mar 11 07:19:16 PST 2016
        SP filesystem version: 0.1.22

        hypervisor_version = Hypervisor 1.10.7.h 2016/03/11 07:13
        obp_version = OpenBoot 4.33.6.g 2016/03/11 06:05
        post_version = POST 4.33.6.g 2016/03/11 06:15
        status = OpenBSD running
        sysfw_version = Sun System Firmware 7.4.10.a 2016/03/11 07:45

The diff below allows a guest to boot from softraid on this machine.

ok?

Index: disk.h
===================================================================
RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/disk.h,v
retrieving revision 1.1
diff -u -p -r1.1 disk.h
--- disk.h 26 Nov 2014 20:30:41 -0000 1.1
+++ disk.h 4 Mar 2018 15:56:59 -0000
@@ -37,6 +37,9 @@ struct diskinfo {
  char path[256];
  struct disklabel disklabel;
  struct sr_boot_volume *sr_vol;
+ int ihandle;
+ int flags;
+#define DISKINFO_FLAG_OPEN 0x1
 
  TAILQ_ENTRY(diskinfo) list;
 };
Index: ofdev.c
===================================================================
RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/ofdev.c,v
retrieving revision 1.25
diff -u -p -r1.25 ofdev.c
--- ofdev.c 1 Oct 2015 16:08:20 -0000 1.25
+++ ofdev.c 4 Mar 2018 15:56:59 -0000
@@ -165,6 +165,7 @@ devclose(struct open_file *of)
 #endif
 #ifdef SOFTRAID
  if (op->type == OFDEV_SOFTRAID) {
+ sr_close_volume(bootdev_dip->sr_vol);
  op->handle = -1;
  return 0;
  }
Index: softraid_sparc64.c
===================================================================
RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/softraid_sparc64.c,v
retrieving revision 1.2
diff -u -p -r1.2 softraid_sparc64.c
--- softraid_sparc64.c 11 Sep 2016 17:53:26 -0000 1.2
+++ softraid_sparc64.c 4 Mar 2018 15:56:59 -0000
@@ -306,6 +306,22 @@ srprobe(void)
  free(md, SR_META_SIZE * DEV_BSIZE);
 }
 
+/*
+ * Cache ihandle to work around memory leaks in firmware
+ * with repeated OF_open/OF_close calls.
+ */
+int
+sr_open_disk(struct diskinfo *dip)
+{
+ if ((dip->flags & DISKINFO_FLAG_OPEN) == 0) {
+ dip->ihandle = OF_open(dip->path);
+ if (dip->ihandle != -1)
+ dip->flags |= DISKINFO_FLAG_OPEN;
+ }
+
+ return dip->ihandle;
+}
+
 int
 sr_strategy(struct sr_boot_volume *bv, int rw, daddr32_t blk, size_t size,
     void *buf, size_t *rsize)
@@ -347,7 +363,7 @@ sr_strategy(struct sr_boot_volume *bv, i
  blk += bv->sbv_data_blkno;
 
  /* XXX - If I/O failed we should try another chunk... */
- ihandle = OF_open(dip->path);
+ ihandle = sr_open_disk(dip);
  if (ihandle == -1)
  return EIO;
  bzero(&ofdev, sizeof(ofdev));
@@ -356,7 +372,6 @@ sr_strategy(struct sr_boot_volume *bv, i
  ofdev.bsize = DEV_BSIZE;
  ofdev.partoff = DL_GETPOFFSET(pp);
  err = strategy(&ofdev, rw, blk, size, buf, rsize);
- OF_close(ihandle);
  return err;
 
  } else if (bv->sbv_level == 'C') {
@@ -371,7 +386,7 @@ sr_strategy(struct sr_boot_volume *bv, i
  dip = (struct diskinfo *)bc->sbc_diskinfo;
  pp = &dip->disklabel.d_partitions[bc->sbc_part - 'a'];
 
- ihandle = OF_open(dip->path);
+ ihandle = sr_open_disk(dip);
  if (ihandle == -1)
  return EIO;
  bzero(&ofdev, sizeof(ofdev));
@@ -395,7 +410,6 @@ sr_strategy(struct sr_boot_volume *bv, i
  printf("Read from crypto volume failed "
     "(read %d bytes): %s\n", *rsize,
     strerror(err));
- OF_close(ihandle);
  return err;
  }
  bcopy(&blkno, iv, sizeof(blkno));
@@ -404,11 +418,30 @@ sr_strategy(struct sr_boot_volume *bv, i
  aes_xts_decrypt(&ctx, bp + j);
  }
  *rsize = nsect * DEV_BSIZE;
- OF_close(ihandle);
  return err;
 
  } else
  return ENOTSUP;
+}
+
+void
+sr_close_volume(struct sr_boot_volume *bv)
+{
+ struct sr_boot_chunk *bc;
+ struct diskinfo *dip;
+
+ /* Select first online chunk. */
+ SLIST_FOREACH(bc, &bv->sbv_chunks, sbc_link)
+ if (bc->sbc_state == BIOC_SDONLINE)
+ break;
+ if (bc == NULL)
+ return;
+
+ dip = (struct diskinfo *)bc->sbc_diskinfo;
+ if (dip && (dip->flags & DISKINFO_FLAG_OPEN)) {
+ OF_close(dip->ihandle);
+ dip->flags &= ~DISKINFO_FLAG_OPEN;
+ }
 }
 
 const char *
Index: softraid_sparc64.h
===================================================================
RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/softraid_sparc64.h,v
retrieving revision 1.3
diff -u -p -r1.3 softraid_sparc64.h
--- softraid_sparc64.h 11 Sep 2016 17:53:26 -0000 1.3
+++ softraid_sparc64.h 4 Mar 2018 15:56:59 -0000
@@ -24,5 +24,6 @@ void srprobe(void);
 const char *sr_getdisklabel(struct sr_boot_volume *, struct disklabel *);
 int sr_strategy(struct sr_boot_volume *, int, daddr32_t, size_t,
     void *, size_t *);
+void sr_close_volume(struct sr_boot_volume *);
 
 #endif /* _SOFTRAID_SPARC64_H */

Reply | Threaded
Open this post in threaded view
|

Re: sparc64 softraid boot: workaround for memory leak

Stefan Sperling-5
On Sun, Mar 04, 2018 at 05:02:45PM +0100, Stefan Sperling wrote:

> On my T5220 LDOM guests cannot boot from softraid because ofwboot
> crashes with a "Fast Data Access MMU Miss" while loading the kernel:
>
> >> OpenBSD BOOT 1.9                                      
> ERROR: /iscsi-hba: No iscsi-network-bootpath property                      
> sr0*                                            
> Passphrase:                          
> Booting sr0:a/bsd          
> 8480888@0x1000000
> ERROR: Last Trap: Fast Data Access MMU Miss
>
> I've tracked down the failure to a crash in OF_open() called in sr_strategy().
> There is no missing OF_close() call. So it seems a memory/resource leak
> of some kind is happening in firmware during OF_open()/OF_close().
>
> Affected firmware version info:
>
> SP firmware 3.0.12.8.a
> SP firmware build number: 108523
> SP firmware date: Fri Mar 11 07:19:16 PST 2016
> SP filesystem version: 0.1.22
>
>         hypervisor_version = Hypervisor 1.10.7.h 2016/03/11 07:13
>         obp_version = OpenBoot 4.33.6.g 2016/03/11 06:05
>         post_version = POST 4.33.6.g 2016/03/11 06:15
>         status = OpenBSD running
>         sysfw_version = Sun System Firmware 7.4.10.a 2016/03/11 07:45
>
> The diff below allows a guest to boot from softraid on this machine.

I haven't had any feedback on the previous diff. However, I felt it
was a bit of a hack, so I tried to come up with a cleaner solution.

The diff below opens the softraid boot chunk early on and keeps the
handle open until a kernel has been loaded from the softraid volume.
This means sr_strategy() does not have to worry about obtaining a
handle from the firmware anymore.

Also consolidate some repeated lines of code in sr_strategy().

Tested on a T5220 ldom guest (CRYPTO) and a v240 machine (RAID1).

Index: boot.c
===================================================================
RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/boot.c,v
retrieving revision 1.27
diff -u -p -r1.27 boot.c
--- boot.c 11 Sep 2016 17:53:26 -0000 1.27
+++ boot.c 12 Mar 2018 10:26:23 -0000
@@ -248,6 +248,8 @@ loadfile(int fd, char *args)
  close(fd);
 
 #ifdef SOFTRAID
+ if (bootdev_dip)
+ OF_close(bootdev_dip->sr_handle);
  sr_clear_keys();
 #endif
  chain(entry, args, ssym, esym);
@@ -283,12 +285,11 @@ fail:
 }
 
 #ifdef SOFTRAID
-/* Set bootdev_dip to the software boot volume, if specified. */
+/* Set bootdev_dip to the softraid boot volume, if specified. */
 static int
 srbootdev(const char *bootline)
 {
  struct sr_boot_volume *bv;
- struct diskinfo *dip;
  int unit;
 
  bootdev_dip = NULL;
@@ -320,9 +321,23 @@ srbootdev(const char *bootline)
  return EPERM;
 
  if (bv->sbv_diskinfo == NULL) {
+ struct sr_boot_chunk *bc;
+ struct diskinfo *dip, *bc_dip;
+ int sr_handle;
+
+ /* All reads will come from the boot chunk. */
+ bc = sr_vol_boot_chunk(bv);
+ if (bc == NULL)
+ return ENXIO;
+ bc_dip = (struct diskinfo *)bc->sbc_diskinfo;
+ sr_handle = OF_open(bc_dip->path);
+ if (sr_handle == -1)
+ return EIO;
+
  dip = alloc(sizeof(struct diskinfo));
  bzero(dip, sizeof(*dip));
  dip->sr_vol = bv;
+ dip->sr_handle = sr_handle;
  bv->sbv_diskinfo = dip;
  }
 
@@ -331,7 +346,8 @@ srbootdev(const char *bootline)
 
  /* Attempt to read disklabel. */
  bv->sbv_part = 'c';
- if (sr_getdisklabel(bv, &dip->disklabel)) {
+ if (sr_getdisklabel(bv, &bootdev_dip->disklabel)) {
+ OF_close(bootdev_dip->sr_handle);
  free(bv->sbv_diskinfo, sizeof(struct diskinfo));
  bv->sbv_diskinfo = NULL;
  bootdev_dip = NULL;
Index: disk.h
===================================================================
RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/disk.h,v
retrieving revision 1.1
diff -u -p -r1.1 disk.h
--- disk.h 26 Nov 2014 20:30:41 -0000 1.1
+++ disk.h 12 Mar 2018 10:26:23 -0000
@@ -36,7 +36,10 @@
 struct diskinfo {
  char path[256];
  struct disklabel disklabel;
+
+ /* Used during softraid boot. */
  struct sr_boot_volume *sr_vol;
+ int sr_handle; /* open handle for reading from boot chunk */
 
  TAILQ_ENTRY(diskinfo) list;
 };
Index: ofdev.c
===================================================================
RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/ofdev.c,v
retrieving revision 1.25
diff -u -p -r1.25 ofdev.c
--- ofdev.c 1 Oct 2015 16:08:20 -0000 1.25
+++ ofdev.c 12 Mar 2018 10:26:23 -0000
@@ -125,8 +125,8 @@ strategy(void *devdata, int rw, daddr32_
 #ifdef SOFTRAID
  /* Intercept strategy for softraid volumes. */
  if (dev->type == OFDEV_SOFTRAID)
- return sr_strategy(bootdev_dip->sr_vol, rw,
-    blk, size, buf, rsize);
+ return sr_strategy(bootdev_dip->sr_vol, bootdev_dip->sr_handle,
+    rw, blk, size, buf, rsize);
 #endif
  if (dev->type != OFDEV_DISK)
  panic("strategy");
Index: softraid_sparc64.c
===================================================================
RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/softraid_sparc64.c,v
retrieving revision 1.2
diff -u -p -r1.2 softraid_sparc64.c
--- softraid_sparc64.c 11 Sep 2016 17:53:26 -0000 1.2
+++ softraid_sparc64.c 12 Mar 2018 10:26:23 -0000
@@ -306,9 +306,25 @@ srprobe(void)
  free(md, SR_META_SIZE * DEV_BSIZE);
 }
 
+struct sr_boot_chunk *
+sr_vol_boot_chunk(struct sr_boot_volume *bv)
+{
+ struct sr_boot_chunk *bc = NULL;
+
+ if (bv->sbv_level == 1 || bv->sbv_level == 'C' ) { /* RAID1 or CRYPTO */
+ /* Select first online chunk. */
+ SLIST_FOREACH(bc, &bv->sbv_chunks, sbc_link)
+ if (bc->sbc_state == BIOC_SDONLINE)
+ break;
+ }
+
+ return bc;
+}
+
+
 int
-sr_strategy(struct sr_boot_volume *bv, int rw, daddr32_t blk, size_t size,
-    void *buf, size_t *rsize)
+sr_strategy(struct sr_boot_volume *bv, int sr_handle, int rw, daddr32_t blk,
+    size_t size, void *buf, size_t *rsize)
 {
  struct diskinfo *sr_dip, *dip;
  struct partition *pp;
@@ -320,7 +336,6 @@ sr_strategy(struct sr_boot_volume *bv, i
  u_char iv[8];
  u_char *bp;
  int err;
- int ihandle;
 
  /* We only support read-only softraid. */
  if (rw != F_READ)
@@ -331,55 +346,28 @@ sr_strategy(struct sr_boot_volume *bv, i
  blk +=
     DL_GETPOFFSET(&sr_dip->disklabel.d_partitions[bv->sbv_part - 'a']);
 
+ bc = sr_vol_boot_chunk(bv);
+ if (bc == NULL)
+ return ENXIO;
+
+ dip = (struct diskinfo *)bc->sbc_diskinfo;
+ pp = &dip->disklabel.d_partitions[bc->sbc_part - 'a'];
+ bzero(&ofdev, sizeof(ofdev));
+ ofdev.handle = sr_handle;
+ ofdev.type = OFDEV_DISK;
+ ofdev.bsize = DEV_BSIZE;
+ ofdev.partoff = DL_GETPOFFSET(pp);
+
  if (bv->sbv_level == 0) {
  return ENOTSUP;
  } else if (bv->sbv_level == 1) {
-
- /* Select first online chunk. */
- SLIST_FOREACH(bc, &bv->sbv_chunks, sbc_link)
- if (bc->sbc_state == BIOC_SDONLINE)
- break;
- if (bc == NULL)
- return EIO;
-
- dip = (struct diskinfo *)bc->sbc_diskinfo;
- pp = &dip->disklabel.d_partitions[bc->sbc_part - 'a'];
  blk += bv->sbv_data_blkno;
 
  /* XXX - If I/O failed we should try another chunk... */
- ihandle = OF_open(dip->path);
- if (ihandle == -1)
- return EIO;
- bzero(&ofdev, sizeof(ofdev));
- ofdev.handle = ihandle;
- ofdev.type = OFDEV_DISK;
- ofdev.bsize = DEV_BSIZE;
- ofdev.partoff = DL_GETPOFFSET(pp);
  err = strategy(&ofdev, rw, blk, size, buf, rsize);
- OF_close(ihandle);
  return err;
 
  } else if (bv->sbv_level == 'C') {
-
- /* Select first online chunk. */
- SLIST_FOREACH(bc, &bv->sbv_chunks, sbc_link)
- if (bc->sbc_state == BIOC_SDONLINE)
- break;
- if (bc == NULL)
- return EIO;
-
- dip = (struct diskinfo *)bc->sbc_diskinfo;
- pp = &dip->disklabel.d_partitions[bc->sbc_part - 'a'];
-
- ihandle = OF_open(dip->path);
- if (ihandle == -1)
- return EIO;
- bzero(&ofdev, sizeof(ofdev));
- ofdev.handle = ihandle;
- ofdev.type = OFDEV_DISK;
- ofdev.bsize = DEV_BSIZE;
- ofdev.partoff = DL_GETPOFFSET(pp);
-
  /* XXX - select correct key. */
  aes_xts_setkey(&ctx, (u_char *)bv->sbv_keys, 64);
 
@@ -395,7 +383,6 @@ sr_strategy(struct sr_boot_volume *bv, i
  printf("Read from crypto volume failed "
     "(read %d bytes): %s\n", *rsize,
     strerror(err));
- OF_close(ihandle);
  return err;
  }
  bcopy(&blkno, iv, sizeof(blkno));
@@ -404,7 +391,6 @@ sr_strategy(struct sr_boot_volume *bv, i
  aes_xts_decrypt(&ctx, bp + j);
  }
  *rsize = nsect * DEV_BSIZE;
- OF_close(ihandle);
  return err;
 
  } else
Index: softraid_sparc64.h
===================================================================
RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/softraid_sparc64.h,v
retrieving revision 1.3
diff -u -p -r1.3 softraid_sparc64.h
--- softraid_sparc64.h 11 Sep 2016 17:53:26 -0000 1.3
+++ softraid_sparc64.h 12 Mar 2018 10:26:23 -0000
@@ -21,8 +21,9 @@
 
 void srprobe(void);
 
+struct sr_boot_chunk *sr_vol_boot_chunk(struct sr_boot_volume *);
 const char *sr_getdisklabel(struct sr_boot_volume *, struct disklabel *);
-int sr_strategy(struct sr_boot_volume *, int, daddr32_t, size_t,
+int sr_strategy(struct sr_boot_volume *, int, int, daddr32_t, size_t,
     void *, size_t *);
 
 #endif /* _SOFTRAID_SPARC64_H */
Index: vers.c
===================================================================
RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/vers.c,v
retrieving revision 1.10
diff -u -p -r1.10 vers.c
--- vers.c 18 Sep 2016 16:36:09 -0000 1.10
+++ vers.c 12 Mar 2018 10:26:23 -0000
@@ -1 +1 @@
-const char version[] = "1.9";
+const char version[] = "1.10";

Reply | Threaded
Open this post in threaded view
|

Re: sparc64 softraid boot: workaround for memory leak

Stefan Sperling-5
On Mon, Mar 12, 2018 at 11:38:14AM +0100, Stefan Sperling wrote:
> I haven't had any feedback on the previous diff. However, I felt it
> was a bit of a hack, so I tried to come up with a cleaner solution.

Anyone? Can this go in now? I hope to get this tested across
many sparc64 machines during the 6.4 release cycle.

> The diff below opens the softraid boot chunk early on and keeps the
> handle open until a kernel has been loaded from the softraid volume.
> This means sr_strategy() does not have to worry about obtaining a
> handle from the firmware anymore.
>
> Also consolidate some repeated lines of code in sr_strategy().
>
> Tested on a T5220 ldom guest (CRYPTO) and a v240 machine (RAID1).
>
> Index: boot.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/boot.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 boot.c
> --- boot.c 11 Sep 2016 17:53:26 -0000 1.27
> +++ boot.c 12 Mar 2018 10:26:23 -0000
> @@ -248,6 +248,8 @@ loadfile(int fd, char *args)
>   close(fd);
>  
>  #ifdef SOFTRAID
> + if (bootdev_dip)
> + OF_close(bootdev_dip->sr_handle);
>   sr_clear_keys();
>  #endif
>   chain(entry, args, ssym, esym);
> @@ -283,12 +285,11 @@ fail:
>  }
>  
>  #ifdef SOFTRAID
> -/* Set bootdev_dip to the software boot volume, if specified. */
> +/* Set bootdev_dip to the softraid boot volume, if specified. */
>  static int
>  srbootdev(const char *bootline)
>  {
>   struct sr_boot_volume *bv;
> - struct diskinfo *dip;
>   int unit;
>  
>   bootdev_dip = NULL;
> @@ -320,9 +321,23 @@ srbootdev(const char *bootline)
>   return EPERM;
>  
>   if (bv->sbv_diskinfo == NULL) {
> + struct sr_boot_chunk *bc;
> + struct diskinfo *dip, *bc_dip;
> + int sr_handle;
> +
> + /* All reads will come from the boot chunk. */
> + bc = sr_vol_boot_chunk(bv);
> + if (bc == NULL)
> + return ENXIO;
> + bc_dip = (struct diskinfo *)bc->sbc_diskinfo;
> + sr_handle = OF_open(bc_dip->path);
> + if (sr_handle == -1)
> + return EIO;
> +
>   dip = alloc(sizeof(struct diskinfo));
>   bzero(dip, sizeof(*dip));
>   dip->sr_vol = bv;
> + dip->sr_handle = sr_handle;
>   bv->sbv_diskinfo = dip;
>   }
>  
> @@ -331,7 +346,8 @@ srbootdev(const char *bootline)
>  
>   /* Attempt to read disklabel. */
>   bv->sbv_part = 'c';
> - if (sr_getdisklabel(bv, &dip->disklabel)) {
> + if (sr_getdisklabel(bv, &bootdev_dip->disklabel)) {
> + OF_close(bootdev_dip->sr_handle);
>   free(bv->sbv_diskinfo, sizeof(struct diskinfo));
>   bv->sbv_diskinfo = NULL;
>   bootdev_dip = NULL;
> Index: disk.h
> ===================================================================
> RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/disk.h,v
> retrieving revision 1.1
> diff -u -p -r1.1 disk.h
> --- disk.h 26 Nov 2014 20:30:41 -0000 1.1
> +++ disk.h 12 Mar 2018 10:26:23 -0000
> @@ -36,7 +36,10 @@
>  struct diskinfo {
>   char path[256];
>   struct disklabel disklabel;
> +
> + /* Used during softraid boot. */
>   struct sr_boot_volume *sr_vol;
> + int sr_handle; /* open handle for reading from boot chunk */
>  
>   TAILQ_ENTRY(diskinfo) list;
>  };
> Index: ofdev.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/ofdev.c,v
> retrieving revision 1.25
> diff -u -p -r1.25 ofdev.c
> --- ofdev.c 1 Oct 2015 16:08:20 -0000 1.25
> +++ ofdev.c 12 Mar 2018 10:26:23 -0000
> @@ -125,8 +125,8 @@ strategy(void *devdata, int rw, daddr32_
>  #ifdef SOFTRAID
>   /* Intercept strategy for softraid volumes. */
>   if (dev->type == OFDEV_SOFTRAID)
> - return sr_strategy(bootdev_dip->sr_vol, rw,
> -    blk, size, buf, rsize);
> + return sr_strategy(bootdev_dip->sr_vol, bootdev_dip->sr_handle,
> +    rw, blk, size, buf, rsize);
>  #endif
>   if (dev->type != OFDEV_DISK)
>   panic("strategy");
> Index: softraid_sparc64.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/softraid_sparc64.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 softraid_sparc64.c
> --- softraid_sparc64.c 11 Sep 2016 17:53:26 -0000 1.2
> +++ softraid_sparc64.c 12 Mar 2018 10:26:23 -0000
> @@ -306,9 +306,25 @@ srprobe(void)
>   free(md, SR_META_SIZE * DEV_BSIZE);
>  }
>  
> +struct sr_boot_chunk *
> +sr_vol_boot_chunk(struct sr_boot_volume *bv)
> +{
> + struct sr_boot_chunk *bc = NULL;
> +
> + if (bv->sbv_level == 1 || bv->sbv_level == 'C' ) { /* RAID1 or CRYPTO */
> + /* Select first online chunk. */
> + SLIST_FOREACH(bc, &bv->sbv_chunks, sbc_link)
> + if (bc->sbc_state == BIOC_SDONLINE)
> + break;
> + }
> +
> + return bc;
> +}
> +
> +
>  int
> -sr_strategy(struct sr_boot_volume *bv, int rw, daddr32_t blk, size_t size,
> -    void *buf, size_t *rsize)
> +sr_strategy(struct sr_boot_volume *bv, int sr_handle, int rw, daddr32_t blk,
> +    size_t size, void *buf, size_t *rsize)
>  {
>   struct diskinfo *sr_dip, *dip;
>   struct partition *pp;
> @@ -320,7 +336,6 @@ sr_strategy(struct sr_boot_volume *bv, i
>   u_char iv[8];
>   u_char *bp;
>   int err;
> - int ihandle;
>  
>   /* We only support read-only softraid. */
>   if (rw != F_READ)
> @@ -331,55 +346,28 @@ sr_strategy(struct sr_boot_volume *bv, i
>   blk +=
>      DL_GETPOFFSET(&sr_dip->disklabel.d_partitions[bv->sbv_part - 'a']);
>  
> + bc = sr_vol_boot_chunk(bv);
> + if (bc == NULL)
> + return ENXIO;
> +
> + dip = (struct diskinfo *)bc->sbc_diskinfo;
> + pp = &dip->disklabel.d_partitions[bc->sbc_part - 'a'];
> + bzero(&ofdev, sizeof(ofdev));
> + ofdev.handle = sr_handle;
> + ofdev.type = OFDEV_DISK;
> + ofdev.bsize = DEV_BSIZE;
> + ofdev.partoff = DL_GETPOFFSET(pp);
> +
>   if (bv->sbv_level == 0) {
>   return ENOTSUP;
>   } else if (bv->sbv_level == 1) {
> -
> - /* Select first online chunk. */
> - SLIST_FOREACH(bc, &bv->sbv_chunks, sbc_link)
> - if (bc->sbc_state == BIOC_SDONLINE)
> - break;
> - if (bc == NULL)
> - return EIO;
> -
> - dip = (struct diskinfo *)bc->sbc_diskinfo;
> - pp = &dip->disklabel.d_partitions[bc->sbc_part - 'a'];
>   blk += bv->sbv_data_blkno;
>  
>   /* XXX - If I/O failed we should try another chunk... */
> - ihandle = OF_open(dip->path);
> - if (ihandle == -1)
> - return EIO;
> - bzero(&ofdev, sizeof(ofdev));
> - ofdev.handle = ihandle;
> - ofdev.type = OFDEV_DISK;
> - ofdev.bsize = DEV_BSIZE;
> - ofdev.partoff = DL_GETPOFFSET(pp);
>   err = strategy(&ofdev, rw, blk, size, buf, rsize);
> - OF_close(ihandle);
>   return err;
>  
>   } else if (bv->sbv_level == 'C') {
> -
> - /* Select first online chunk. */
> - SLIST_FOREACH(bc, &bv->sbv_chunks, sbc_link)
> - if (bc->sbc_state == BIOC_SDONLINE)
> - break;
> - if (bc == NULL)
> - return EIO;
> -
> - dip = (struct diskinfo *)bc->sbc_diskinfo;
> - pp = &dip->disklabel.d_partitions[bc->sbc_part - 'a'];
> -
> - ihandle = OF_open(dip->path);
> - if (ihandle == -1)
> - return EIO;
> - bzero(&ofdev, sizeof(ofdev));
> - ofdev.handle = ihandle;
> - ofdev.type = OFDEV_DISK;
> - ofdev.bsize = DEV_BSIZE;
> - ofdev.partoff = DL_GETPOFFSET(pp);
> -
>   /* XXX - select correct key. */
>   aes_xts_setkey(&ctx, (u_char *)bv->sbv_keys, 64);
>  
> @@ -395,7 +383,6 @@ sr_strategy(struct sr_boot_volume *bv, i
>   printf("Read from crypto volume failed "
>      "(read %d bytes): %s\n", *rsize,
>      strerror(err));
> - OF_close(ihandle);
>   return err;
>   }
>   bcopy(&blkno, iv, sizeof(blkno));
> @@ -404,7 +391,6 @@ sr_strategy(struct sr_boot_volume *bv, i
>   aes_xts_decrypt(&ctx, bp + j);
>   }
>   *rsize = nsect * DEV_BSIZE;
> - OF_close(ihandle);
>   return err;
>  
>   } else
> Index: softraid_sparc64.h
> ===================================================================
> RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/softraid_sparc64.h,v
> retrieving revision 1.3
> diff -u -p -r1.3 softraid_sparc64.h
> --- softraid_sparc64.h 11 Sep 2016 17:53:26 -0000 1.3
> +++ softraid_sparc64.h 12 Mar 2018 10:26:23 -0000
> @@ -21,8 +21,9 @@
>  
>  void srprobe(void);
>  
> +struct sr_boot_chunk *sr_vol_boot_chunk(struct sr_boot_volume *);
>  const char *sr_getdisklabel(struct sr_boot_volume *, struct disklabel *);
> -int sr_strategy(struct sr_boot_volume *, int, daddr32_t, size_t,
> +int sr_strategy(struct sr_boot_volume *, int, int, daddr32_t, size_t,
>      void *, size_t *);
>  
>  #endif /* _SOFTRAID_SPARC64_H */
> Index: vers.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/vers.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 vers.c
> --- vers.c 18 Sep 2016 16:36:09 -0000 1.10
> +++ vers.c 12 Mar 2018 10:26:23 -0000
> @@ -1 +1 @@
> -const char version[] = "1.9";
> +const char version[] = "1.10";
>

Reply | Threaded
Open this post in threaded view
|

Re: sparc64 softraid boot: workaround for memory leak

Mike Larkin
On Thu, Mar 29, 2018 at 08:40:27AM +0200, Stefan Sperling wrote:
> On Mon, Mar 12, 2018 at 11:38:14AM +0100, Stefan Sperling wrote:
> > I haven't had any feedback on the previous diff. However, I felt it
> > was a bit of a hack, so I tried to come up with a cleaner solution.
>
> Anyone? Can this go in now? I hope to get this tested across
> many sparc64 machines during the 6.4 release cycle.
>

Wish I could help but my T5240 has never been able to run any ldoms. Something
about mismatched firmware versions. ldomctl(8) just generates interrupt
storms.

-ml

> > The diff below opens the softraid boot chunk early on and keeps the
> > handle open until a kernel has been loaded from the softraid volume.
> > This means sr_strategy() does not have to worry about obtaining a
> > handle from the firmware anymore.
> >
> > Also consolidate some repeated lines of code in sr_strategy().
> >
> > Tested on a T5220 ldom guest (CRYPTO) and a v240 machine (RAID1).
> >
> > Index: boot.c
> > ===================================================================
> > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/boot.c,v
> > retrieving revision 1.27
> > diff -u -p -r1.27 boot.c
> > --- boot.c 11 Sep 2016 17:53:26 -0000 1.27
> > +++ boot.c 12 Mar 2018 10:26:23 -0000
> > @@ -248,6 +248,8 @@ loadfile(int fd, char *args)
> >   close(fd);
> >  
> >  #ifdef SOFTRAID
> > + if (bootdev_dip)
> > + OF_close(bootdev_dip->sr_handle);
> >   sr_clear_keys();
> >  #endif
> >   chain(entry, args, ssym, esym);
> > @@ -283,12 +285,11 @@ fail:
> >  }
> >  
> >  #ifdef SOFTRAID
> > -/* Set bootdev_dip to the software boot volume, if specified. */
> > +/* Set bootdev_dip to the softraid boot volume, if specified. */
> >  static int
> >  srbootdev(const char *bootline)
> >  {
> >   struct sr_boot_volume *bv;
> > - struct diskinfo *dip;
> >   int unit;
> >  
> >   bootdev_dip = NULL;
> > @@ -320,9 +321,23 @@ srbootdev(const char *bootline)
> >   return EPERM;
> >  
> >   if (bv->sbv_diskinfo == NULL) {
> > + struct sr_boot_chunk *bc;
> > + struct diskinfo *dip, *bc_dip;
> > + int sr_handle;
> > +
> > + /* All reads will come from the boot chunk. */
> > + bc = sr_vol_boot_chunk(bv);
> > + if (bc == NULL)
> > + return ENXIO;
> > + bc_dip = (struct diskinfo *)bc->sbc_diskinfo;
> > + sr_handle = OF_open(bc_dip->path);
> > + if (sr_handle == -1)
> > + return EIO;
> > +
> >   dip = alloc(sizeof(struct diskinfo));
> >   bzero(dip, sizeof(*dip));
> >   dip->sr_vol = bv;
> > + dip->sr_handle = sr_handle;
> >   bv->sbv_diskinfo = dip;
> >   }
> >  
> > @@ -331,7 +346,8 @@ srbootdev(const char *bootline)
> >  
> >   /* Attempt to read disklabel. */
> >   bv->sbv_part = 'c';
> > - if (sr_getdisklabel(bv, &dip->disklabel)) {
> > + if (sr_getdisklabel(bv, &bootdev_dip->disklabel)) {
> > + OF_close(bootdev_dip->sr_handle);
> >   free(bv->sbv_diskinfo, sizeof(struct diskinfo));
> >   bv->sbv_diskinfo = NULL;
> >   bootdev_dip = NULL;
> > Index: disk.h
> > ===================================================================
> > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/disk.h,v
> > retrieving revision 1.1
> > diff -u -p -r1.1 disk.h
> > --- disk.h 26 Nov 2014 20:30:41 -0000 1.1
> > +++ disk.h 12 Mar 2018 10:26:23 -0000
> > @@ -36,7 +36,10 @@
> >  struct diskinfo {
> >   char path[256];
> >   struct disklabel disklabel;
> > +
> > + /* Used during softraid boot. */
> >   struct sr_boot_volume *sr_vol;
> > + int sr_handle; /* open handle for reading from boot chunk */
> >  
> >   TAILQ_ENTRY(diskinfo) list;
> >  };
> > Index: ofdev.c
> > ===================================================================
> > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/ofdev.c,v
> > retrieving revision 1.25
> > diff -u -p -r1.25 ofdev.c
> > --- ofdev.c 1 Oct 2015 16:08:20 -0000 1.25
> > +++ ofdev.c 12 Mar 2018 10:26:23 -0000
> > @@ -125,8 +125,8 @@ strategy(void *devdata, int rw, daddr32_
> >  #ifdef SOFTRAID
> >   /* Intercept strategy for softraid volumes. */
> >   if (dev->type == OFDEV_SOFTRAID)
> > - return sr_strategy(bootdev_dip->sr_vol, rw,
> > -    blk, size, buf, rsize);
> > + return sr_strategy(bootdev_dip->sr_vol, bootdev_dip->sr_handle,
> > +    rw, blk, size, buf, rsize);
> >  #endif
> >   if (dev->type != OFDEV_DISK)
> >   panic("strategy");
> > Index: softraid_sparc64.c
> > ===================================================================
> > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/softraid_sparc64.c,v
> > retrieving revision 1.2
> > diff -u -p -r1.2 softraid_sparc64.c
> > --- softraid_sparc64.c 11 Sep 2016 17:53:26 -0000 1.2
> > +++ softraid_sparc64.c 12 Mar 2018 10:26:23 -0000
> > @@ -306,9 +306,25 @@ srprobe(void)
> >   free(md, SR_META_SIZE * DEV_BSIZE);
> >  }
> >  
> > +struct sr_boot_chunk *
> > +sr_vol_boot_chunk(struct sr_boot_volume *bv)
> > +{
> > + struct sr_boot_chunk *bc = NULL;
> > +
> > + if (bv->sbv_level == 1 || bv->sbv_level == 'C' ) { /* RAID1 or CRYPTO */
> > + /* Select first online chunk. */
> > + SLIST_FOREACH(bc, &bv->sbv_chunks, sbc_link)
> > + if (bc->sbc_state == BIOC_SDONLINE)
> > + break;
> > + }
> > +
> > + return bc;
> > +}
> > +
> > +
> >  int
> > -sr_strategy(struct sr_boot_volume *bv, int rw, daddr32_t blk, size_t size,
> > -    void *buf, size_t *rsize)
> > +sr_strategy(struct sr_boot_volume *bv, int sr_handle, int rw, daddr32_t blk,
> > +    size_t size, void *buf, size_t *rsize)
> >  {
> >   struct diskinfo *sr_dip, *dip;
> >   struct partition *pp;
> > @@ -320,7 +336,6 @@ sr_strategy(struct sr_boot_volume *bv, i
> >   u_char iv[8];
> >   u_char *bp;
> >   int err;
> > - int ihandle;
> >  
> >   /* We only support read-only softraid. */
> >   if (rw != F_READ)
> > @@ -331,55 +346,28 @@ sr_strategy(struct sr_boot_volume *bv, i
> >   blk +=
> >      DL_GETPOFFSET(&sr_dip->disklabel.d_partitions[bv->sbv_part - 'a']);
> >  
> > + bc = sr_vol_boot_chunk(bv);
> > + if (bc == NULL)
> > + return ENXIO;
> > +
> > + dip = (struct diskinfo *)bc->sbc_diskinfo;
> > + pp = &dip->disklabel.d_partitions[bc->sbc_part - 'a'];
> > + bzero(&ofdev, sizeof(ofdev));
> > + ofdev.handle = sr_handle;
> > + ofdev.type = OFDEV_DISK;
> > + ofdev.bsize = DEV_BSIZE;
> > + ofdev.partoff = DL_GETPOFFSET(pp);
> > +
> >   if (bv->sbv_level == 0) {
> >   return ENOTSUP;
> >   } else if (bv->sbv_level == 1) {
> > -
> > - /* Select first online chunk. */
> > - SLIST_FOREACH(bc, &bv->sbv_chunks, sbc_link)
> > - if (bc->sbc_state == BIOC_SDONLINE)
> > - break;
> > - if (bc == NULL)
> > - return EIO;
> > -
> > - dip = (struct diskinfo *)bc->sbc_diskinfo;
> > - pp = &dip->disklabel.d_partitions[bc->sbc_part - 'a'];
> >   blk += bv->sbv_data_blkno;
> >  
> >   /* XXX - If I/O failed we should try another chunk... */
> > - ihandle = OF_open(dip->path);
> > - if (ihandle == -1)
> > - return EIO;
> > - bzero(&ofdev, sizeof(ofdev));
> > - ofdev.handle = ihandle;
> > - ofdev.type = OFDEV_DISK;
> > - ofdev.bsize = DEV_BSIZE;
> > - ofdev.partoff = DL_GETPOFFSET(pp);
> >   err = strategy(&ofdev, rw, blk, size, buf, rsize);
> > - OF_close(ihandle);
> >   return err;
> >  
> >   } else if (bv->sbv_level == 'C') {
> > -
> > - /* Select first online chunk. */
> > - SLIST_FOREACH(bc, &bv->sbv_chunks, sbc_link)
> > - if (bc->sbc_state == BIOC_SDONLINE)
> > - break;
> > - if (bc == NULL)
> > - return EIO;
> > -
> > - dip = (struct diskinfo *)bc->sbc_diskinfo;
> > - pp = &dip->disklabel.d_partitions[bc->sbc_part - 'a'];
> > -
> > - ihandle = OF_open(dip->path);
> > - if (ihandle == -1)
> > - return EIO;
> > - bzero(&ofdev, sizeof(ofdev));
> > - ofdev.handle = ihandle;
> > - ofdev.type = OFDEV_DISK;
> > - ofdev.bsize = DEV_BSIZE;
> > - ofdev.partoff = DL_GETPOFFSET(pp);
> > -
> >   /* XXX - select correct key. */
> >   aes_xts_setkey(&ctx, (u_char *)bv->sbv_keys, 64);
> >  
> > @@ -395,7 +383,6 @@ sr_strategy(struct sr_boot_volume *bv, i
> >   printf("Read from crypto volume failed "
> >      "(read %d bytes): %s\n", *rsize,
> >      strerror(err));
> > - OF_close(ihandle);
> >   return err;
> >   }
> >   bcopy(&blkno, iv, sizeof(blkno));
> > @@ -404,7 +391,6 @@ sr_strategy(struct sr_boot_volume *bv, i
> >   aes_xts_decrypt(&ctx, bp + j);
> >   }
> >   *rsize = nsect * DEV_BSIZE;
> > - OF_close(ihandle);
> >   return err;
> >  
> >   } else
> > Index: softraid_sparc64.h
> > ===================================================================
> > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/softraid_sparc64.h,v
> > retrieving revision 1.3
> > diff -u -p -r1.3 softraid_sparc64.h
> > --- softraid_sparc64.h 11 Sep 2016 17:53:26 -0000 1.3
> > +++ softraid_sparc64.h 12 Mar 2018 10:26:23 -0000
> > @@ -21,8 +21,9 @@
> >  
> >  void srprobe(void);
> >  
> > +struct sr_boot_chunk *sr_vol_boot_chunk(struct sr_boot_volume *);
> >  const char *sr_getdisklabel(struct sr_boot_volume *, struct disklabel *);
> > -int sr_strategy(struct sr_boot_volume *, int, daddr32_t, size_t,
> > +int sr_strategy(struct sr_boot_volume *, int, int, daddr32_t, size_t,
> >      void *, size_t *);
> >  
> >  #endif /* _SOFTRAID_SPARC64_H */
> > Index: vers.c
> > ===================================================================
> > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/vers.c,v
> > retrieving revision 1.10
> > diff -u -p -r1.10 vers.c
> > --- vers.c 18 Sep 2016 16:36:09 -0000 1.10
> > +++ vers.c 12 Mar 2018 10:26:23 -0000
> > @@ -1 +1 @@
> > -const char version[] = "1.9";
> > +const char version[] = "1.10";
> >
>

Reply | Threaded
Open this post in threaded view
|

Re: sparc64 softraid boot: workaround for memory leak

Mark Kettenis
In reply to this post by Stefan Sperling-5
> From: Stefan Sperling <[hidden email]>
>
> On Mon, Mar 12, 2018 at 11:38:14AM +0100, Stefan Sperling wrote:
> > I haven't had any feedback on the previous diff. However, I felt it
> > was a bit of a hack, so I tried to come up with a cleaner solution.
>
> Anyone? Can this go in now? I hope to get this tested across
> many sparc64 machines during the 6.4 release cycle.

ok kettenis@

> > The diff below opens the softraid boot chunk early on and keeps the
> > handle open until a kernel has been loaded from the softraid volume.
> > This means sr_strategy() does not have to worry about obtaining a
> > handle from the firmware anymore.
> >
> > Also consolidate some repeated lines of code in sr_strategy().
> >
> > Tested on a T5220 ldom guest (CRYPTO) and a v240 machine (RAID1).
> >
> > Index: boot.c
> > ===================================================================
> > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/boot.c,v
> > retrieving revision 1.27
> > diff -u -p -r1.27 boot.c
> > --- boot.c 11 Sep 2016 17:53:26 -0000 1.27
> > +++ boot.c 12 Mar 2018 10:26:23 -0000
> > @@ -248,6 +248,8 @@ loadfile(int fd, char *args)
> >   close(fd);
> >  
> >  #ifdef SOFTRAID
> > + if (bootdev_dip)
> > + OF_close(bootdev_dip->sr_handle);
> >   sr_clear_keys();
> >  #endif
> >   chain(entry, args, ssym, esym);
> > @@ -283,12 +285,11 @@ fail:
> >  }
> >  
> >  #ifdef SOFTRAID
> > -/* Set bootdev_dip to the software boot volume, if specified. */
> > +/* Set bootdev_dip to the softraid boot volume, if specified. */
> >  static int
> >  srbootdev(const char *bootline)
> >  {
> >   struct sr_boot_volume *bv;
> > - struct diskinfo *dip;
> >   int unit;
> >  
> >   bootdev_dip = NULL;
> > @@ -320,9 +321,23 @@ srbootdev(const char *bootline)
> >   return EPERM;
> >  
> >   if (bv->sbv_diskinfo == NULL) {
> > + struct sr_boot_chunk *bc;
> > + struct diskinfo *dip, *bc_dip;
> > + int sr_handle;
> > +
> > + /* All reads will come from the boot chunk. */
> > + bc = sr_vol_boot_chunk(bv);
> > + if (bc == NULL)
> > + return ENXIO;
> > + bc_dip = (struct diskinfo *)bc->sbc_diskinfo;
> > + sr_handle = OF_open(bc_dip->path);
> > + if (sr_handle == -1)
> > + return EIO;
> > +
> >   dip = alloc(sizeof(struct diskinfo));
> >   bzero(dip, sizeof(*dip));
> >   dip->sr_vol = bv;
> > + dip->sr_handle = sr_handle;
> >   bv->sbv_diskinfo = dip;
> >   }
> >  
> > @@ -331,7 +346,8 @@ srbootdev(const char *bootline)
> >  
> >   /* Attempt to read disklabel. */
> >   bv->sbv_part = 'c';
> > - if (sr_getdisklabel(bv, &dip->disklabel)) {
> > + if (sr_getdisklabel(bv, &bootdev_dip->disklabel)) {
> > + OF_close(bootdev_dip->sr_handle);
> >   free(bv->sbv_diskinfo, sizeof(struct diskinfo));
> >   bv->sbv_diskinfo = NULL;
> >   bootdev_dip = NULL;
> > Index: disk.h
> > ===================================================================
> > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/disk.h,v
> > retrieving revision 1.1
> > diff -u -p -r1.1 disk.h
> > --- disk.h 26 Nov 2014 20:30:41 -0000 1.1
> > +++ disk.h 12 Mar 2018 10:26:23 -0000
> > @@ -36,7 +36,10 @@
> >  struct diskinfo {
> >   char path[256];
> >   struct disklabel disklabel;
> > +
> > + /* Used during softraid boot. */
> >   struct sr_boot_volume *sr_vol;
> > + int sr_handle; /* open handle for reading from boot chunk */
> >  
> >   TAILQ_ENTRY(diskinfo) list;
> >  };
> > Index: ofdev.c
> > ===================================================================
> > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/ofdev.c,v
> > retrieving revision 1.25
> > diff -u -p -r1.25 ofdev.c
> > --- ofdev.c 1 Oct 2015 16:08:20 -0000 1.25
> > +++ ofdev.c 12 Mar 2018 10:26:23 -0000
> > @@ -125,8 +125,8 @@ strategy(void *devdata, int rw, daddr32_
> >  #ifdef SOFTRAID
> >   /* Intercept strategy for softraid volumes. */
> >   if (dev->type == OFDEV_SOFTRAID)
> > - return sr_strategy(bootdev_dip->sr_vol, rw,
> > -    blk, size, buf, rsize);
> > + return sr_strategy(bootdev_dip->sr_vol, bootdev_dip->sr_handle,
> > +    rw, blk, size, buf, rsize);
> >  #endif
> >   if (dev->type != OFDEV_DISK)
> >   panic("strategy");
> > Index: softraid_sparc64.c
> > ===================================================================
> > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/softraid_sparc64.c,v
> > retrieving revision 1.2
> > diff -u -p -r1.2 softraid_sparc64.c
> > --- softraid_sparc64.c 11 Sep 2016 17:53:26 -0000 1.2
> > +++ softraid_sparc64.c 12 Mar 2018 10:26:23 -0000
> > @@ -306,9 +306,25 @@ srprobe(void)
> >   free(md, SR_META_SIZE * DEV_BSIZE);
> >  }
> >  
> > +struct sr_boot_chunk *
> > +sr_vol_boot_chunk(struct sr_boot_volume *bv)
> > +{
> > + struct sr_boot_chunk *bc = NULL;
> > +
> > + if (bv->sbv_level == 1 || bv->sbv_level == 'C' ) { /* RAID1 or CRYPTO */
> > + /* Select first online chunk. */
> > + SLIST_FOREACH(bc, &bv->sbv_chunks, sbc_link)
> > + if (bc->sbc_state == BIOC_SDONLINE)
> > + break;
> > + }
> > +
> > + return bc;
> > +}
> > +
> > +
> >  int
> > -sr_strategy(struct sr_boot_volume *bv, int rw, daddr32_t blk, size_t size,
> > -    void *buf, size_t *rsize)
> > +sr_strategy(struct sr_boot_volume *bv, int sr_handle, int rw, daddr32_t blk,
> > +    size_t size, void *buf, size_t *rsize)
> >  {
> >   struct diskinfo *sr_dip, *dip;
> >   struct partition *pp;
> > @@ -320,7 +336,6 @@ sr_strategy(struct sr_boot_volume *bv, i
> >   u_char iv[8];
> >   u_char *bp;
> >   int err;
> > - int ihandle;
> >  
> >   /* We only support read-only softraid. */
> >   if (rw != F_READ)
> > @@ -331,55 +346,28 @@ sr_strategy(struct sr_boot_volume *bv, i
> >   blk +=
> >      DL_GETPOFFSET(&sr_dip->disklabel.d_partitions[bv->sbv_part - 'a']);
> >  
> > + bc = sr_vol_boot_chunk(bv);
> > + if (bc == NULL)
> > + return ENXIO;
> > +
> > + dip = (struct diskinfo *)bc->sbc_diskinfo;
> > + pp = &dip->disklabel.d_partitions[bc->sbc_part - 'a'];
> > + bzero(&ofdev, sizeof(ofdev));
> > + ofdev.handle = sr_handle;
> > + ofdev.type = OFDEV_DISK;
> > + ofdev.bsize = DEV_BSIZE;
> > + ofdev.partoff = DL_GETPOFFSET(pp);
> > +
> >   if (bv->sbv_level == 0) {
> >   return ENOTSUP;
> >   } else if (bv->sbv_level == 1) {
> > -
> > - /* Select first online chunk. */
> > - SLIST_FOREACH(bc, &bv->sbv_chunks, sbc_link)
> > - if (bc->sbc_state == BIOC_SDONLINE)
> > - break;
> > - if (bc == NULL)
> > - return EIO;
> > -
> > - dip = (struct diskinfo *)bc->sbc_diskinfo;
> > - pp = &dip->disklabel.d_partitions[bc->sbc_part - 'a'];
> >   blk += bv->sbv_data_blkno;
> >  
> >   /* XXX - If I/O failed we should try another chunk... */
> > - ihandle = OF_open(dip->path);
> > - if (ihandle == -1)
> > - return EIO;
> > - bzero(&ofdev, sizeof(ofdev));
> > - ofdev.handle = ihandle;
> > - ofdev.type = OFDEV_DISK;
> > - ofdev.bsize = DEV_BSIZE;
> > - ofdev.partoff = DL_GETPOFFSET(pp);
> >   err = strategy(&ofdev, rw, blk, size, buf, rsize);
> > - OF_close(ihandle);
> >   return err;
> >  
> >   } else if (bv->sbv_level == 'C') {
> > -
> > - /* Select first online chunk. */
> > - SLIST_FOREACH(bc, &bv->sbv_chunks, sbc_link)
> > - if (bc->sbc_state == BIOC_SDONLINE)
> > - break;
> > - if (bc == NULL)
> > - return EIO;
> > -
> > - dip = (struct diskinfo *)bc->sbc_diskinfo;
> > - pp = &dip->disklabel.d_partitions[bc->sbc_part - 'a'];
> > -
> > - ihandle = OF_open(dip->path);
> > - if (ihandle == -1)
> > - return EIO;
> > - bzero(&ofdev, sizeof(ofdev));
> > - ofdev.handle = ihandle;
> > - ofdev.type = OFDEV_DISK;
> > - ofdev.bsize = DEV_BSIZE;
> > - ofdev.partoff = DL_GETPOFFSET(pp);
> > -
> >   /* XXX - select correct key. */
> >   aes_xts_setkey(&ctx, (u_char *)bv->sbv_keys, 64);
> >  
> > @@ -395,7 +383,6 @@ sr_strategy(struct sr_boot_volume *bv, i
> >   printf("Read from crypto volume failed "
> >      "(read %d bytes): %s\n", *rsize,
> >      strerror(err));
> > - OF_close(ihandle);
> >   return err;
> >   }
> >   bcopy(&blkno, iv, sizeof(blkno));
> > @@ -404,7 +391,6 @@ sr_strategy(struct sr_boot_volume *bv, i
> >   aes_xts_decrypt(&ctx, bp + j);
> >   }
> >   *rsize = nsect * DEV_BSIZE;
> > - OF_close(ihandle);
> >   return err;
> >  
> >   } else
> > Index: softraid_sparc64.h
> > ===================================================================
> > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/softraid_sparc64.h,v
> > retrieving revision 1.3
> > diff -u -p -r1.3 softraid_sparc64.h
> > --- softraid_sparc64.h 11 Sep 2016 17:53:26 -0000 1.3
> > +++ softraid_sparc64.h 12 Mar 2018 10:26:23 -0000
> > @@ -21,8 +21,9 @@
> >  
> >  void srprobe(void);
> >  
> > +struct sr_boot_chunk *sr_vol_boot_chunk(struct sr_boot_volume *);
> >  const char *sr_getdisklabel(struct sr_boot_volume *, struct disklabel *);
> > -int sr_strategy(struct sr_boot_volume *, int, daddr32_t, size_t,
> > +int sr_strategy(struct sr_boot_volume *, int, int, daddr32_t, size_t,
> >      void *, size_t *);
> >  
> >  #endif /* _SOFTRAID_SPARC64_H */
> > Index: vers.c
> > ===================================================================
> > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/vers.c,v
> > retrieving revision 1.10
> > diff -u -p -r1.10 vers.c
> > --- vers.c 18 Sep 2016 16:36:09 -0000 1.10
> > +++ vers.c 12 Mar 2018 10:26:23 -0000
> > @@ -1 +1 @@
> > -const char version[] = "1.9";
> > +const char version[] = "1.10";
> >
>
>

Reply | Threaded
Open this post in threaded view
|

Re: sparc64 softraid boot: workaround for memory leak

Mark Kettenis
In reply to this post by Mike Larkin
> Date: Wed, 28 Mar 2018 23:46:49 -0700
> From: Mike Larkin <[hidden email]>
>
> On Thu, Mar 29, 2018 at 08:40:27AM +0200, Stefan Sperling wrote:
> > On Mon, Mar 12, 2018 at 11:38:14AM +0100, Stefan Sperling wrote:
> > > I haven't had any feedback on the previous diff. However, I felt it
> > > was a bit of a hack, so I tried to come up with a cleaner solution.
> >
> > Anyone? Can this go in now? I hope to get this tested across
> > many sparc64 machines during the 6.4 release cycle.
> >
>
> Wish I could help but my T5240 has never been able to run any ldoms. Something
> about mismatched firmware versions. ldomctl(8) just generates interrupt
> storms.

Even with all the fixes that stsp@ made during the 6.3 release cycle?

> > > The diff below opens the softraid boot chunk early on and keeps the
> > > handle open until a kernel has been loaded from the softraid volume.
> > > This means sr_strategy() does not have to worry about obtaining a
> > > handle from the firmware anymore.
> > >
> > > Also consolidate some repeated lines of code in sr_strategy().
> > >
> > > Tested on a T5220 ldom guest (CRYPTO) and a v240 machine (RAID1).
> > >
> > > Index: boot.c
> > > ===================================================================
> > > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/boot.c,v
> > > retrieving revision 1.27
> > > diff -u -p -r1.27 boot.c
> > > --- boot.c 11 Sep 2016 17:53:26 -0000 1.27
> > > +++ boot.c 12 Mar 2018 10:26:23 -0000
> > > @@ -248,6 +248,8 @@ loadfile(int fd, char *args)
> > >   close(fd);
> > >  
> > >  #ifdef SOFTRAID
> > > + if (bootdev_dip)
> > > + OF_close(bootdev_dip->sr_handle);
> > >   sr_clear_keys();
> > >  #endif
> > >   chain(entry, args, ssym, esym);
> > > @@ -283,12 +285,11 @@ fail:
> > >  }
> > >  
> > >  #ifdef SOFTRAID
> > > -/* Set bootdev_dip to the software boot volume, if specified. */
> > > +/* Set bootdev_dip to the softraid boot volume, if specified. */
> > >  static int
> > >  srbootdev(const char *bootline)
> > >  {
> > >   struct sr_boot_volume *bv;
> > > - struct diskinfo *dip;
> > >   int unit;
> > >  
> > >   bootdev_dip = NULL;
> > > @@ -320,9 +321,23 @@ srbootdev(const char *bootline)
> > >   return EPERM;
> > >  
> > >   if (bv->sbv_diskinfo == NULL) {
> > > + struct sr_boot_chunk *bc;
> > > + struct diskinfo *dip, *bc_dip;
> > > + int sr_handle;
> > > +
> > > + /* All reads will come from the boot chunk. */
> > > + bc = sr_vol_boot_chunk(bv);
> > > + if (bc == NULL)
> > > + return ENXIO;
> > > + bc_dip = (struct diskinfo *)bc->sbc_diskinfo;
> > > + sr_handle = OF_open(bc_dip->path);
> > > + if (sr_handle == -1)
> > > + return EIO;
> > > +
> > >   dip = alloc(sizeof(struct diskinfo));
> > >   bzero(dip, sizeof(*dip));
> > >   dip->sr_vol = bv;
> > > + dip->sr_handle = sr_handle;
> > >   bv->sbv_diskinfo = dip;
> > >   }
> > >  
> > > @@ -331,7 +346,8 @@ srbootdev(const char *bootline)
> > >  
> > >   /* Attempt to read disklabel. */
> > >   bv->sbv_part = 'c';
> > > - if (sr_getdisklabel(bv, &dip->disklabel)) {
> > > + if (sr_getdisklabel(bv, &bootdev_dip->disklabel)) {
> > > + OF_close(bootdev_dip->sr_handle);
> > >   free(bv->sbv_diskinfo, sizeof(struct diskinfo));
> > >   bv->sbv_diskinfo = NULL;
> > >   bootdev_dip = NULL;
> > > Index: disk.h
> > > ===================================================================
> > > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/disk.h,v
> > > retrieving revision 1.1
> > > diff -u -p -r1.1 disk.h
> > > --- disk.h 26 Nov 2014 20:30:41 -0000 1.1
> > > +++ disk.h 12 Mar 2018 10:26:23 -0000
> > > @@ -36,7 +36,10 @@
> > >  struct diskinfo {
> > >   char path[256];
> > >   struct disklabel disklabel;
> > > +
> > > + /* Used during softraid boot. */
> > >   struct sr_boot_volume *sr_vol;
> > > + int sr_handle; /* open handle for reading from boot chunk */
> > >  
> > >   TAILQ_ENTRY(diskinfo) list;
> > >  };
> > > Index: ofdev.c
> > > ===================================================================
> > > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/ofdev.c,v
> > > retrieving revision 1.25
> > > diff -u -p -r1.25 ofdev.c
> > > --- ofdev.c 1 Oct 2015 16:08:20 -0000 1.25
> > > +++ ofdev.c 12 Mar 2018 10:26:23 -0000
> > > @@ -125,8 +125,8 @@ strategy(void *devdata, int rw, daddr32_
> > >  #ifdef SOFTRAID
> > >   /* Intercept strategy for softraid volumes. */
> > >   if (dev->type == OFDEV_SOFTRAID)
> > > - return sr_strategy(bootdev_dip->sr_vol, rw,
> > > -    blk, size, buf, rsize);
> > > + return sr_strategy(bootdev_dip->sr_vol, bootdev_dip->sr_handle,
> > > +    rw, blk, size, buf, rsize);
> > >  #endif
> > >   if (dev->type != OFDEV_DISK)
> > >   panic("strategy");
> > > Index: softraid_sparc64.c
> > > ===================================================================
> > > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/softraid_sparc64.c,v
> > > retrieving revision 1.2
> > > diff -u -p -r1.2 softraid_sparc64.c
> > > --- softraid_sparc64.c 11 Sep 2016 17:53:26 -0000 1.2
> > > +++ softraid_sparc64.c 12 Mar 2018 10:26:23 -0000
> > > @@ -306,9 +306,25 @@ srprobe(void)
> > >   free(md, SR_META_SIZE * DEV_BSIZE);
> > >  }
> > >  
> > > +struct sr_boot_chunk *
> > > +sr_vol_boot_chunk(struct sr_boot_volume *bv)
> > > +{
> > > + struct sr_boot_chunk *bc = NULL;
> > > +
> > > + if (bv->sbv_level == 1 || bv->sbv_level == 'C' ) { /* RAID1 or CRYPTO */
> > > + /* Select first online chunk. */
> > > + SLIST_FOREACH(bc, &bv->sbv_chunks, sbc_link)
> > > + if (bc->sbc_state == BIOC_SDONLINE)
> > > + break;
> > > + }
> > > +
> > > + return bc;
> > > +}
> > > +
> > > +
> > >  int
> > > -sr_strategy(struct sr_boot_volume *bv, int rw, daddr32_t blk, size_t size,
> > > -    void *buf, size_t *rsize)
> > > +sr_strategy(struct sr_boot_volume *bv, int sr_handle, int rw, daddr32_t blk,
> > > +    size_t size, void *buf, size_t *rsize)
> > >  {
> > >   struct diskinfo *sr_dip, *dip;
> > >   struct partition *pp;
> > > @@ -320,7 +336,6 @@ sr_strategy(struct sr_boot_volume *bv, i
> > >   u_char iv[8];
> > >   u_char *bp;
> > >   int err;
> > > - int ihandle;
> > >  
> > >   /* We only support read-only softraid. */
> > >   if (rw != F_READ)
> > > @@ -331,55 +346,28 @@ sr_strategy(struct sr_boot_volume *bv, i
> > >   blk +=
> > >      DL_GETPOFFSET(&sr_dip->disklabel.d_partitions[bv->sbv_part - 'a']);
> > >  
> > > + bc = sr_vol_boot_chunk(bv);
> > > + if (bc == NULL)
> > > + return ENXIO;
> > > +
> > > + dip = (struct diskinfo *)bc->sbc_diskinfo;
> > > + pp = &dip->disklabel.d_partitions[bc->sbc_part - 'a'];
> > > + bzero(&ofdev, sizeof(ofdev));
> > > + ofdev.handle = sr_handle;
> > > + ofdev.type = OFDEV_DISK;
> > > + ofdev.bsize = DEV_BSIZE;
> > > + ofdev.partoff = DL_GETPOFFSET(pp);
> > > +
> > >   if (bv->sbv_level == 0) {
> > >   return ENOTSUP;
> > >   } else if (bv->sbv_level == 1) {
> > > -
> > > - /* Select first online chunk. */
> > > - SLIST_FOREACH(bc, &bv->sbv_chunks, sbc_link)
> > > - if (bc->sbc_state == BIOC_SDONLINE)
> > > - break;
> > > - if (bc == NULL)
> > > - return EIO;
> > > -
> > > - dip = (struct diskinfo *)bc->sbc_diskinfo;
> > > - pp = &dip->disklabel.d_partitions[bc->sbc_part - 'a'];
> > >   blk += bv->sbv_data_blkno;
> > >  
> > >   /* XXX - If I/O failed we should try another chunk... */
> > > - ihandle = OF_open(dip->path);
> > > - if (ihandle == -1)
> > > - return EIO;
> > > - bzero(&ofdev, sizeof(ofdev));
> > > - ofdev.handle = ihandle;
> > > - ofdev.type = OFDEV_DISK;
> > > - ofdev.bsize = DEV_BSIZE;
> > > - ofdev.partoff = DL_GETPOFFSET(pp);
> > >   err = strategy(&ofdev, rw, blk, size, buf, rsize);
> > > - OF_close(ihandle);
> > >   return err;
> > >  
> > >   } else if (bv->sbv_level == 'C') {
> > > -
> > > - /* Select first online chunk. */
> > > - SLIST_FOREACH(bc, &bv->sbv_chunks, sbc_link)
> > > - if (bc->sbc_state == BIOC_SDONLINE)
> > > - break;
> > > - if (bc == NULL)
> > > - return EIO;
> > > -
> > > - dip = (struct diskinfo *)bc->sbc_diskinfo;
> > > - pp = &dip->disklabel.d_partitions[bc->sbc_part - 'a'];
> > > -
> > > - ihandle = OF_open(dip->path);
> > > - if (ihandle == -1)
> > > - return EIO;
> > > - bzero(&ofdev, sizeof(ofdev));
> > > - ofdev.handle = ihandle;
> > > - ofdev.type = OFDEV_DISK;
> > > - ofdev.bsize = DEV_BSIZE;
> > > - ofdev.partoff = DL_GETPOFFSET(pp);
> > > -
> > >   /* XXX - select correct key. */
> > >   aes_xts_setkey(&ctx, (u_char *)bv->sbv_keys, 64);
> > >  
> > > @@ -395,7 +383,6 @@ sr_strategy(struct sr_boot_volume *bv, i
> > >   printf("Read from crypto volume failed "
> > >      "(read %d bytes): %s\n", *rsize,
> > >      strerror(err));
> > > - OF_close(ihandle);
> > >   return err;
> > >   }
> > >   bcopy(&blkno, iv, sizeof(blkno));
> > > @@ -404,7 +391,6 @@ sr_strategy(struct sr_boot_volume *bv, i
> > >   aes_xts_decrypt(&ctx, bp + j);
> > >   }
> > >   *rsize = nsect * DEV_BSIZE;
> > > - OF_close(ihandle);
> > >   return err;
> > >  
> > >   } else
> > > Index: softraid_sparc64.h
> > > ===================================================================
> > > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/softraid_sparc64.h,v
> > > retrieving revision 1.3
> > > diff -u -p -r1.3 softraid_sparc64.h
> > > --- softraid_sparc64.h 11 Sep 2016 17:53:26 -0000 1.3
> > > +++ softraid_sparc64.h 12 Mar 2018 10:26:23 -0000
> > > @@ -21,8 +21,9 @@
> > >  
> > >  void srprobe(void);
> > >  
> > > +struct sr_boot_chunk *sr_vol_boot_chunk(struct sr_boot_volume *);
> > >  const char *sr_getdisklabel(struct sr_boot_volume *, struct disklabel *);
> > > -int sr_strategy(struct sr_boot_volume *, int, daddr32_t, size_t,
> > > +int sr_strategy(struct sr_boot_volume *, int, int, daddr32_t, size_t,
> > >      void *, size_t *);
> > >  
> > >  #endif /* _SOFTRAID_SPARC64_H */
> > > Index: vers.c
> > > ===================================================================
> > > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/vers.c,v
> > > retrieving revision 1.10
> > > diff -u -p -r1.10 vers.c
> > > --- vers.c 18 Sep 2016 16:36:09 -0000 1.10
> > > +++ vers.c 12 Mar 2018 10:26:23 -0000
> > > @@ -1 +1 @@
> > > -const char version[] = "1.9";
> > > +const char version[] = "1.10";
> > >
> >
>
>

Reply | Threaded
Open this post in threaded view
|

Re: sparc64 softraid boot: workaround for memory leak

Mike Larkin
On Thu, Mar 29, 2018 at 09:21:16AM +0200, Mark Kettenis wrote:

> > Date: Wed, 28 Mar 2018 23:46:49 -0700
> > From: Mike Larkin <[hidden email]>
> >
> > On Thu, Mar 29, 2018 at 08:40:27AM +0200, Stefan Sperling wrote:
> > > On Mon, Mar 12, 2018 at 11:38:14AM +0100, Stefan Sperling wrote:
> > > > I haven't had any feedback on the previous diff. However, I felt it
> > > > was a bit of a hack, so I tried to come up with a cleaner solution.
> > >
> > > Anyone? Can this go in now? I hope to get this tested across
> > > many sparc64 machines during the 6.4 release cycle.
> > >
> >
> > Wish I could help but my T5240 has never been able to run any ldoms. Something
> > about mismatched firmware versions. ldomctl(8) just generates interrupt
> > storms.
>
> Even with all the fixes that stsp@ made during the 6.3 release cycle?
>

As of about a month ago, I still couldn't run any ldoms. But since you mention
it, I'll try again tomorrow!

-ml

> > > > The diff below opens the softraid boot chunk early on and keeps the
> > > > handle open until a kernel has been loaded from the softraid volume.
> > > > This means sr_strategy() does not have to worry about obtaining a
> > > > handle from the firmware anymore.
> > > >
> > > > Also consolidate some repeated lines of code in sr_strategy().
> > > >
> > > > Tested on a T5220 ldom guest (CRYPTO) and a v240 machine (RAID1).
> > > >
> > > > Index: boot.c
> > > > ===================================================================
> > > > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/boot.c,v
> > > > retrieving revision 1.27
> > > > diff -u -p -r1.27 boot.c
> > > > --- boot.c 11 Sep 2016 17:53:26 -0000 1.27
> > > > +++ boot.c 12 Mar 2018 10:26:23 -0000
> > > > @@ -248,6 +248,8 @@ loadfile(int fd, char *args)
> > > >   close(fd);
> > > >  
> > > >  #ifdef SOFTRAID
> > > > + if (bootdev_dip)
> > > > + OF_close(bootdev_dip->sr_handle);
> > > >   sr_clear_keys();
> > > >  #endif
> > > >   chain(entry, args, ssym, esym);
> > > > @@ -283,12 +285,11 @@ fail:
> > > >  }
> > > >  
> > > >  #ifdef SOFTRAID
> > > > -/* Set bootdev_dip to the software boot volume, if specified. */
> > > > +/* Set bootdev_dip to the softraid boot volume, if specified. */
> > > >  static int
> > > >  srbootdev(const char *bootline)
> > > >  {
> > > >   struct sr_boot_volume *bv;
> > > > - struct diskinfo *dip;
> > > >   int unit;
> > > >  
> > > >   bootdev_dip = NULL;
> > > > @@ -320,9 +321,23 @@ srbootdev(const char *bootline)
> > > >   return EPERM;
> > > >  
> > > >   if (bv->sbv_diskinfo == NULL) {
> > > > + struct sr_boot_chunk *bc;
> > > > + struct diskinfo *dip, *bc_dip;
> > > > + int sr_handle;
> > > > +
> > > > + /* All reads will come from the boot chunk. */
> > > > + bc = sr_vol_boot_chunk(bv);
> > > > + if (bc == NULL)
> > > > + return ENXIO;
> > > > + bc_dip = (struct diskinfo *)bc->sbc_diskinfo;
> > > > + sr_handle = OF_open(bc_dip->path);
> > > > + if (sr_handle == -1)
> > > > + return EIO;
> > > > +
> > > >   dip = alloc(sizeof(struct diskinfo));
> > > >   bzero(dip, sizeof(*dip));
> > > >   dip->sr_vol = bv;
> > > > + dip->sr_handle = sr_handle;
> > > >   bv->sbv_diskinfo = dip;
> > > >   }
> > > >  
> > > > @@ -331,7 +346,8 @@ srbootdev(const char *bootline)
> > > >  
> > > >   /* Attempt to read disklabel. */
> > > >   bv->sbv_part = 'c';
> > > > - if (sr_getdisklabel(bv, &dip->disklabel)) {
> > > > + if (sr_getdisklabel(bv, &bootdev_dip->disklabel)) {
> > > > + OF_close(bootdev_dip->sr_handle);
> > > >   free(bv->sbv_diskinfo, sizeof(struct diskinfo));
> > > >   bv->sbv_diskinfo = NULL;
> > > >   bootdev_dip = NULL;
> > > > Index: disk.h
> > > > ===================================================================
> > > > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/disk.h,v
> > > > retrieving revision 1.1
> > > > diff -u -p -r1.1 disk.h
> > > > --- disk.h 26 Nov 2014 20:30:41 -0000 1.1
> > > > +++ disk.h 12 Mar 2018 10:26:23 -0000
> > > > @@ -36,7 +36,10 @@
> > > >  struct diskinfo {
> > > >   char path[256];
> > > >   struct disklabel disklabel;
> > > > +
> > > > + /* Used during softraid boot. */
> > > >   struct sr_boot_volume *sr_vol;
> > > > + int sr_handle; /* open handle for reading from boot chunk */
> > > >  
> > > >   TAILQ_ENTRY(diskinfo) list;
> > > >  };
> > > > Index: ofdev.c
> > > > ===================================================================
> > > > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/ofdev.c,v
> > > > retrieving revision 1.25
> > > > diff -u -p -r1.25 ofdev.c
> > > > --- ofdev.c 1 Oct 2015 16:08:20 -0000 1.25
> > > > +++ ofdev.c 12 Mar 2018 10:26:23 -0000
> > > > @@ -125,8 +125,8 @@ strategy(void *devdata, int rw, daddr32_
> > > >  #ifdef SOFTRAID
> > > >   /* Intercept strategy for softraid volumes. */
> > > >   if (dev->type == OFDEV_SOFTRAID)
> > > > - return sr_strategy(bootdev_dip->sr_vol, rw,
> > > > -    blk, size, buf, rsize);
> > > > + return sr_strategy(bootdev_dip->sr_vol, bootdev_dip->sr_handle,
> > > > +    rw, blk, size, buf, rsize);
> > > >  #endif
> > > >   if (dev->type != OFDEV_DISK)
> > > >   panic("strategy");
> > > > Index: softraid_sparc64.c
> > > > ===================================================================
> > > > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/softraid_sparc64.c,v
> > > > retrieving revision 1.2
> > > > diff -u -p -r1.2 softraid_sparc64.c
> > > > --- softraid_sparc64.c 11 Sep 2016 17:53:26 -0000 1.2
> > > > +++ softraid_sparc64.c 12 Mar 2018 10:26:23 -0000
> > > > @@ -306,9 +306,25 @@ srprobe(void)
> > > >   free(md, SR_META_SIZE * DEV_BSIZE);
> > > >  }
> > > >  
> > > > +struct sr_boot_chunk *
> > > > +sr_vol_boot_chunk(struct sr_boot_volume *bv)
> > > > +{
> > > > + struct sr_boot_chunk *bc = NULL;
> > > > +
> > > > + if (bv->sbv_level == 1 || bv->sbv_level == 'C' ) { /* RAID1 or CRYPTO */
> > > > + /* Select first online chunk. */
> > > > + SLIST_FOREACH(bc, &bv->sbv_chunks, sbc_link)
> > > > + if (bc->sbc_state == BIOC_SDONLINE)
> > > > + break;
> > > > + }
> > > > +
> > > > + return bc;
> > > > +}
> > > > +
> > > > +
> > > >  int
> > > > -sr_strategy(struct sr_boot_volume *bv, int rw, daddr32_t blk, size_t size,
> > > > -    void *buf, size_t *rsize)
> > > > +sr_strategy(struct sr_boot_volume *bv, int sr_handle, int rw, daddr32_t blk,
> > > > +    size_t size, void *buf, size_t *rsize)
> > > >  {
> > > >   struct diskinfo *sr_dip, *dip;
> > > >   struct partition *pp;
> > > > @@ -320,7 +336,6 @@ sr_strategy(struct sr_boot_volume *bv, i
> > > >   u_char iv[8];
> > > >   u_char *bp;
> > > >   int err;
> > > > - int ihandle;
> > > >  
> > > >   /* We only support read-only softraid. */
> > > >   if (rw != F_READ)
> > > > @@ -331,55 +346,28 @@ sr_strategy(struct sr_boot_volume *bv, i
> > > >   blk +=
> > > >      DL_GETPOFFSET(&sr_dip->disklabel.d_partitions[bv->sbv_part - 'a']);
> > > >  
> > > > + bc = sr_vol_boot_chunk(bv);
> > > > + if (bc == NULL)
> > > > + return ENXIO;
> > > > +
> > > > + dip = (struct diskinfo *)bc->sbc_diskinfo;
> > > > + pp = &dip->disklabel.d_partitions[bc->sbc_part - 'a'];
> > > > + bzero(&ofdev, sizeof(ofdev));
> > > > + ofdev.handle = sr_handle;
> > > > + ofdev.type = OFDEV_DISK;
> > > > + ofdev.bsize = DEV_BSIZE;
> > > > + ofdev.partoff = DL_GETPOFFSET(pp);
> > > > +
> > > >   if (bv->sbv_level == 0) {
> > > >   return ENOTSUP;
> > > >   } else if (bv->sbv_level == 1) {
> > > > -
> > > > - /* Select first online chunk. */
> > > > - SLIST_FOREACH(bc, &bv->sbv_chunks, sbc_link)
> > > > - if (bc->sbc_state == BIOC_SDONLINE)
> > > > - break;
> > > > - if (bc == NULL)
> > > > - return EIO;
> > > > -
> > > > - dip = (struct diskinfo *)bc->sbc_diskinfo;
> > > > - pp = &dip->disklabel.d_partitions[bc->sbc_part - 'a'];
> > > >   blk += bv->sbv_data_blkno;
> > > >  
> > > >   /* XXX - If I/O failed we should try another chunk... */
> > > > - ihandle = OF_open(dip->path);
> > > > - if (ihandle == -1)
> > > > - return EIO;
> > > > - bzero(&ofdev, sizeof(ofdev));
> > > > - ofdev.handle = ihandle;
> > > > - ofdev.type = OFDEV_DISK;
> > > > - ofdev.bsize = DEV_BSIZE;
> > > > - ofdev.partoff = DL_GETPOFFSET(pp);
> > > >   err = strategy(&ofdev, rw, blk, size, buf, rsize);
> > > > - OF_close(ihandle);
> > > >   return err;
> > > >  
> > > >   } else if (bv->sbv_level == 'C') {
> > > > -
> > > > - /* Select first online chunk. */
> > > > - SLIST_FOREACH(bc, &bv->sbv_chunks, sbc_link)
> > > > - if (bc->sbc_state == BIOC_SDONLINE)
> > > > - break;
> > > > - if (bc == NULL)
> > > > - return EIO;
> > > > -
> > > > - dip = (struct diskinfo *)bc->sbc_diskinfo;
> > > > - pp = &dip->disklabel.d_partitions[bc->sbc_part - 'a'];
> > > > -
> > > > - ihandle = OF_open(dip->path);
> > > > - if (ihandle == -1)
> > > > - return EIO;
> > > > - bzero(&ofdev, sizeof(ofdev));
> > > > - ofdev.handle = ihandle;
> > > > - ofdev.type = OFDEV_DISK;
> > > > - ofdev.bsize = DEV_BSIZE;
> > > > - ofdev.partoff = DL_GETPOFFSET(pp);
> > > > -
> > > >   /* XXX - select correct key. */
> > > >   aes_xts_setkey(&ctx, (u_char *)bv->sbv_keys, 64);
> > > >  
> > > > @@ -395,7 +383,6 @@ sr_strategy(struct sr_boot_volume *bv, i
> > > >   printf("Read from crypto volume failed "
> > > >      "(read %d bytes): %s\n", *rsize,
> > > >      strerror(err));
> > > > - OF_close(ihandle);
> > > >   return err;
> > > >   }
> > > >   bcopy(&blkno, iv, sizeof(blkno));
> > > > @@ -404,7 +391,6 @@ sr_strategy(struct sr_boot_volume *bv, i
> > > >   aes_xts_decrypt(&ctx, bp + j);
> > > >   }
> > > >   *rsize = nsect * DEV_BSIZE;
> > > > - OF_close(ihandle);
> > > >   return err;
> > > >  
> > > >   } else
> > > > Index: softraid_sparc64.h
> > > > ===================================================================
> > > > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/softraid_sparc64.h,v
> > > > retrieving revision 1.3
> > > > diff -u -p -r1.3 softraid_sparc64.h
> > > > --- softraid_sparc64.h 11 Sep 2016 17:53:26 -0000 1.3
> > > > +++ softraid_sparc64.h 12 Mar 2018 10:26:23 -0000
> > > > @@ -21,8 +21,9 @@
> > > >  
> > > >  void srprobe(void);
> > > >  
> > > > +struct sr_boot_chunk *sr_vol_boot_chunk(struct sr_boot_volume *);
> > > >  const char *sr_getdisklabel(struct sr_boot_volume *, struct disklabel *);
> > > > -int sr_strategy(struct sr_boot_volume *, int, daddr32_t, size_t,
> > > > +int sr_strategy(struct sr_boot_volume *, int, int, daddr32_t, size_t,
> > > >      void *, size_t *);
> > > >  
> > > >  #endif /* _SOFTRAID_SPARC64_H */
> > > > Index: vers.c
> > > > ===================================================================
> > > > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/vers.c,v
> > > > retrieving revision 1.10
> > > > diff -u -p -r1.10 vers.c
> > > > --- vers.c 18 Sep 2016 16:36:09 -0000 1.10
> > > > +++ vers.c 12 Mar 2018 10:26:23 -0000
> > > > @@ -1 +1 @@
> > > > -const char version[] = "1.9";
> > > > +const char version[] = "1.10";
> > > >
> > >
> >
> >
>

Reply | Threaded
Open this post in threaded view
|

Re: sparc64 softraid boot: workaround for memory leak

Mike Larkin
In reply to this post by Mark Kettenis
On Thu, Mar 29, 2018 at 09:21:16AM +0200, Mark Kettenis wrote:

> > Date: Wed, 28 Mar 2018 23:46:49 -0700
> > From: Mike Larkin <[hidden email]>
> >
> > On Thu, Mar 29, 2018 at 08:40:27AM +0200, Stefan Sperling wrote:
> > > On Mon, Mar 12, 2018 at 11:38:14AM +0100, Stefan Sperling wrote:
> > > > I haven't had any feedback on the previous diff. However, I felt it
> > > > was a bit of a hack, so I tried to come up with a cleaner solution.
> > >
> > > Anyone? Can this go in now? I hope to get this tested across
> > > many sparc64 machines during the 6.4 release cycle.
> > >
> >
> > Wish I could help but my T5240 has never been able to run any ldoms. Something
> > about mismatched firmware versions. ldomctl(8) just generates interrupt
> > storms.
>
> Even with all the fixes that stsp@ made during the 6.3 release cycle?
>

It does look like some things have been fixed. I still can't launch ldoms
but I believe this due to some local error, not anything in the code. All the
commands work now, it's just that after reboot the ldom isn't started.

I'll see if I can diagnose more this week, but that machine is a beast and
really noisy.

-ml

> > > > The diff below opens the softraid boot chunk early on and keeps the
> > > > handle open until a kernel has been loaded from the softraid volume.
> > > > This means sr_strategy() does not have to worry about obtaining a
> > > > handle from the firmware anymore.
> > > >
> > > > Also consolidate some repeated lines of code in sr_strategy().
> > > >
> > > > Tested on a T5220 ldom guest (CRYPTO) and a v240 machine (RAID1).
> > > >
> > > > Index: boot.c
> > > > ===================================================================
> > > > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/boot.c,v
> > > > retrieving revision 1.27
> > > > diff -u -p -r1.27 boot.c
> > > > --- boot.c 11 Sep 2016 17:53:26 -0000 1.27
> > > > +++ boot.c 12 Mar 2018 10:26:23 -0000
> > > > @@ -248,6 +248,8 @@ loadfile(int fd, char *args)
> > > >   close(fd);
> > > >  
> > > >  #ifdef SOFTRAID
> > > > + if (bootdev_dip)
> > > > + OF_close(bootdev_dip->sr_handle);
> > > >   sr_clear_keys();
> > > >  #endif
> > > >   chain(entry, args, ssym, esym);
> > > > @@ -283,12 +285,11 @@ fail:
> > > >  }
> > > >  
> > > >  #ifdef SOFTRAID
> > > > -/* Set bootdev_dip to the software boot volume, if specified. */
> > > > +/* Set bootdev_dip to the softraid boot volume, if specified. */
> > > >  static int
> > > >  srbootdev(const char *bootline)
> > > >  {
> > > >   struct sr_boot_volume *bv;
> > > > - struct diskinfo *dip;
> > > >   int unit;
> > > >  
> > > >   bootdev_dip = NULL;
> > > > @@ -320,9 +321,23 @@ srbootdev(const char *bootline)
> > > >   return EPERM;
> > > >  
> > > >   if (bv->sbv_diskinfo == NULL) {
> > > > + struct sr_boot_chunk *bc;
> > > > + struct diskinfo *dip, *bc_dip;
> > > > + int sr_handle;
> > > > +
> > > > + /* All reads will come from the boot chunk. */
> > > > + bc = sr_vol_boot_chunk(bv);
> > > > + if (bc == NULL)
> > > > + return ENXIO;
> > > > + bc_dip = (struct diskinfo *)bc->sbc_diskinfo;
> > > > + sr_handle = OF_open(bc_dip->path);
> > > > + if (sr_handle == -1)
> > > > + return EIO;
> > > > +
> > > >   dip = alloc(sizeof(struct diskinfo));
> > > >   bzero(dip, sizeof(*dip));
> > > >   dip->sr_vol = bv;
> > > > + dip->sr_handle = sr_handle;
> > > >   bv->sbv_diskinfo = dip;
> > > >   }
> > > >  
> > > > @@ -331,7 +346,8 @@ srbootdev(const char *bootline)
> > > >  
> > > >   /* Attempt to read disklabel. */
> > > >   bv->sbv_part = 'c';
> > > > - if (sr_getdisklabel(bv, &dip->disklabel)) {
> > > > + if (sr_getdisklabel(bv, &bootdev_dip->disklabel)) {
> > > > + OF_close(bootdev_dip->sr_handle);
> > > >   free(bv->sbv_diskinfo, sizeof(struct diskinfo));
> > > >   bv->sbv_diskinfo = NULL;
> > > >   bootdev_dip = NULL;
> > > > Index: disk.h
> > > > ===================================================================
> > > > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/disk.h,v
> > > > retrieving revision 1.1
> > > > diff -u -p -r1.1 disk.h
> > > > --- disk.h 26 Nov 2014 20:30:41 -0000 1.1
> > > > +++ disk.h 12 Mar 2018 10:26:23 -0000
> > > > @@ -36,7 +36,10 @@
> > > >  struct diskinfo {
> > > >   char path[256];
> > > >   struct disklabel disklabel;
> > > > +
> > > > + /* Used during softraid boot. */
> > > >   struct sr_boot_volume *sr_vol;
> > > > + int sr_handle; /* open handle for reading from boot chunk */
> > > >  
> > > >   TAILQ_ENTRY(diskinfo) list;
> > > >  };
> > > > Index: ofdev.c
> > > > ===================================================================
> > > > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/ofdev.c,v
> > > > retrieving revision 1.25
> > > > diff -u -p -r1.25 ofdev.c
> > > > --- ofdev.c 1 Oct 2015 16:08:20 -0000 1.25
> > > > +++ ofdev.c 12 Mar 2018 10:26:23 -0000
> > > > @@ -125,8 +125,8 @@ strategy(void *devdata, int rw, daddr32_
> > > >  #ifdef SOFTRAID
> > > >   /* Intercept strategy for softraid volumes. */
> > > >   if (dev->type == OFDEV_SOFTRAID)
> > > > - return sr_strategy(bootdev_dip->sr_vol, rw,
> > > > -    blk, size, buf, rsize);
> > > > + return sr_strategy(bootdev_dip->sr_vol, bootdev_dip->sr_handle,
> > > > +    rw, blk, size, buf, rsize);
> > > >  #endif
> > > >   if (dev->type != OFDEV_DISK)
> > > >   panic("strategy");
> > > > Index: softraid_sparc64.c
> > > > ===================================================================
> > > > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/softraid_sparc64.c,v
> > > > retrieving revision 1.2
> > > > diff -u -p -r1.2 softraid_sparc64.c
> > > > --- softraid_sparc64.c 11 Sep 2016 17:53:26 -0000 1.2
> > > > +++ softraid_sparc64.c 12 Mar 2018 10:26:23 -0000
> > > > @@ -306,9 +306,25 @@ srprobe(void)
> > > >   free(md, SR_META_SIZE * DEV_BSIZE);
> > > >  }
> > > >  
> > > > +struct sr_boot_chunk *
> > > > +sr_vol_boot_chunk(struct sr_boot_volume *bv)
> > > > +{
> > > > + struct sr_boot_chunk *bc = NULL;
> > > > +
> > > > + if (bv->sbv_level == 1 || bv->sbv_level == 'C' ) { /* RAID1 or CRYPTO */
> > > > + /* Select first online chunk. */
> > > > + SLIST_FOREACH(bc, &bv->sbv_chunks, sbc_link)
> > > > + if (bc->sbc_state == BIOC_SDONLINE)
> > > > + break;
> > > > + }
> > > > +
> > > > + return bc;
> > > > +}
> > > > +
> > > > +
> > > >  int
> > > > -sr_strategy(struct sr_boot_volume *bv, int rw, daddr32_t blk, size_t size,
> > > > -    void *buf, size_t *rsize)
> > > > +sr_strategy(struct sr_boot_volume *bv, int sr_handle, int rw, daddr32_t blk,
> > > > +    size_t size, void *buf, size_t *rsize)
> > > >  {
> > > >   struct diskinfo *sr_dip, *dip;
> > > >   struct partition *pp;
> > > > @@ -320,7 +336,6 @@ sr_strategy(struct sr_boot_volume *bv, i
> > > >   u_char iv[8];
> > > >   u_char *bp;
> > > >   int err;
> > > > - int ihandle;
> > > >  
> > > >   /* We only support read-only softraid. */
> > > >   if (rw != F_READ)
> > > > @@ -331,55 +346,28 @@ sr_strategy(struct sr_boot_volume *bv, i
> > > >   blk +=
> > > >      DL_GETPOFFSET(&sr_dip->disklabel.d_partitions[bv->sbv_part - 'a']);
> > > >  
> > > > + bc = sr_vol_boot_chunk(bv);
> > > > + if (bc == NULL)
> > > > + return ENXIO;
> > > > +
> > > > + dip = (struct diskinfo *)bc->sbc_diskinfo;
> > > > + pp = &dip->disklabel.d_partitions[bc->sbc_part - 'a'];
> > > > + bzero(&ofdev, sizeof(ofdev));
> > > > + ofdev.handle = sr_handle;
> > > > + ofdev.type = OFDEV_DISK;
> > > > + ofdev.bsize = DEV_BSIZE;
> > > > + ofdev.partoff = DL_GETPOFFSET(pp);
> > > > +
> > > >   if (bv->sbv_level == 0) {
> > > >   return ENOTSUP;
> > > >   } else if (bv->sbv_level == 1) {
> > > > -
> > > > - /* Select first online chunk. */
> > > > - SLIST_FOREACH(bc, &bv->sbv_chunks, sbc_link)
> > > > - if (bc->sbc_state == BIOC_SDONLINE)
> > > > - break;
> > > > - if (bc == NULL)
> > > > - return EIO;
> > > > -
> > > > - dip = (struct diskinfo *)bc->sbc_diskinfo;
> > > > - pp = &dip->disklabel.d_partitions[bc->sbc_part - 'a'];
> > > >   blk += bv->sbv_data_blkno;
> > > >  
> > > >   /* XXX - If I/O failed we should try another chunk... */
> > > > - ihandle = OF_open(dip->path);
> > > > - if (ihandle == -1)
> > > > - return EIO;
> > > > - bzero(&ofdev, sizeof(ofdev));
> > > > - ofdev.handle = ihandle;
> > > > - ofdev.type = OFDEV_DISK;
> > > > - ofdev.bsize = DEV_BSIZE;
> > > > - ofdev.partoff = DL_GETPOFFSET(pp);
> > > >   err = strategy(&ofdev, rw, blk, size, buf, rsize);
> > > > - OF_close(ihandle);
> > > >   return err;
> > > >  
> > > >   } else if (bv->sbv_level == 'C') {
> > > > -
> > > > - /* Select first online chunk. */
> > > > - SLIST_FOREACH(bc, &bv->sbv_chunks, sbc_link)
> > > > - if (bc->sbc_state == BIOC_SDONLINE)
> > > > - break;
> > > > - if (bc == NULL)
> > > > - return EIO;
> > > > -
> > > > - dip = (struct diskinfo *)bc->sbc_diskinfo;
> > > > - pp = &dip->disklabel.d_partitions[bc->sbc_part - 'a'];
> > > > -
> > > > - ihandle = OF_open(dip->path);
> > > > - if (ihandle == -1)
> > > > - return EIO;
> > > > - bzero(&ofdev, sizeof(ofdev));
> > > > - ofdev.handle = ihandle;
> > > > - ofdev.type = OFDEV_DISK;
> > > > - ofdev.bsize = DEV_BSIZE;
> > > > - ofdev.partoff = DL_GETPOFFSET(pp);
> > > > -
> > > >   /* XXX - select correct key. */
> > > >   aes_xts_setkey(&ctx, (u_char *)bv->sbv_keys, 64);
> > > >  
> > > > @@ -395,7 +383,6 @@ sr_strategy(struct sr_boot_volume *bv, i
> > > >   printf("Read from crypto volume failed "
> > > >      "(read %d bytes): %s\n", *rsize,
> > > >      strerror(err));
> > > > - OF_close(ihandle);
> > > >   return err;
> > > >   }
> > > >   bcopy(&blkno, iv, sizeof(blkno));
> > > > @@ -404,7 +391,6 @@ sr_strategy(struct sr_boot_volume *bv, i
> > > >   aes_xts_decrypt(&ctx, bp + j);
> > > >   }
> > > >   *rsize = nsect * DEV_BSIZE;
> > > > - OF_close(ihandle);
> > > >   return err;
> > > >  
> > > >   } else
> > > > Index: softraid_sparc64.h
> > > > ===================================================================
> > > > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/softraid_sparc64.h,v
> > > > retrieving revision 1.3
> > > > diff -u -p -r1.3 softraid_sparc64.h
> > > > --- softraid_sparc64.h 11 Sep 2016 17:53:26 -0000 1.3
> > > > +++ softraid_sparc64.h 12 Mar 2018 10:26:23 -0000
> > > > @@ -21,8 +21,9 @@
> > > >  
> > > >  void srprobe(void);
> > > >  
> > > > +struct sr_boot_chunk *sr_vol_boot_chunk(struct sr_boot_volume *);
> > > >  const char *sr_getdisklabel(struct sr_boot_volume *, struct disklabel *);
> > > > -int sr_strategy(struct sr_boot_volume *, int, daddr32_t, size_t,
> > > > +int sr_strategy(struct sr_boot_volume *, int, int, daddr32_t, size_t,
> > > >      void *, size_t *);
> > > >  
> > > >  #endif /* _SOFTRAID_SPARC64_H */
> > > > Index: vers.c
> > > > ===================================================================
> > > > RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/vers.c,v
> > > > retrieving revision 1.10
> > > > diff -u -p -r1.10 vers.c
> > > > --- vers.c 18 Sep 2016 16:36:09 -0000 1.10
> > > > +++ vers.c 12 Mar 2018 10:26:23 -0000
> > > > @@ -1 +1 @@
> > > > -const char version[] = "1.9";
> > > > +const char version[] = "1.10";
> > > >
> > >
> >
> >