Re: athn(4) USB question: Where is Tx interrupt handler?
On Sun, May 08, 2022 at 12:29:57AM -0400, Farhan Khan wrote: > On May 6, 2022 4:37:48 AM EDT, Stefan Sperling wrote: > >On Thu, May 05, 2022 at 01:19:08PM -0400, Farhan Khan wrote: > >> Hi all, > >> > >> Summary Question: > >> > >> Broadly, I am trying to understand where a interrupt callback is specified > >> if > >> not already specified by usbd_open_pipe_intr(9). Specifically, for the > >> athn(4) > >> driver, I am trying to understand if/how athn_usb_intr() is executed for > >> Tx > >> interrupts. This seems necessary yet I do not see a callback specified by > >> usbd_setup_xfer(9) or by usbd_open_pipe_intr(9). > >> > >> All code is located in sys/dev/usb/if_athn_usb.c. > >> > >> Question Walk-through: > >> > >> >From reading the code, it seems that the athn_usb_intr() function is > >> >called > >> whenever a Tx interrupt is triggered. The reason I think this is because > >> there > >> is a tsleep_nsec(9) for a Tx interrupt that awaits for a wakeup(9) that > >> only > >> happens in athn_usb_intr(). > >> > >> The 3 relevant steps are listed below in athn_usb_htc_setup() under the > >> comment "Set credits for WLAN Tx pipe": > >> > >> 1. athn_usb_htc_msg(), which runs usbd_setup_xfer(9) and usbd_transfer(9) > >> for > >> a Tx interrupt. The callback is set to NULL. > >> 2. usc->wait_msg_id is set to AR_HTC_MSG_CONF_PIPE_RSP. > >> 3. A tsleep_nsec() on &usc->wait_msg_id > >> > >> The only place I see a wakeup(9) on &usc->wait_msg_id is within > >> athn_usb_intr(), on condition that usc->wait_msg_id is set to > >> AR_HTC_MSG_CONF_PIPE_RSP. Seems like a perfect match. Additionally, I do > >> not > >> see an Rx interrupt anywhere else. But even if it does happen somewhere > >> and I > >> am just missing it, the only place AR_HTC_MSG_CONF_PIPE_RSP is used is > >> step 2. > >> > >> Rx interrupt callbacks to athn_usb_intr() are specified by the > >> usbd_open_pipe_intr(9) call in athn_usb_open_pipes(). That seems explicit. > >> But > >> for the Tx interrupt, I do not see where the mapping to athn_usb_intr() is. > >> > >> Please assist, thank you. > > > >Everything related to Tx is happening in athn_usb_tx() and athn_usb_txoef(). > > > >usbd_setup_xfer() gets a function pointer to call when the USB transfer > >has completed. This function pointer is athn_usb_txeof(): > > > > usbd_setup_xfer(data->xfer, usc->tx_data_pipe, data, data->buf, > > xferlen, USBD_FORCE_SHORT_XFER | USBD_NO_COPY, ATHN_USB_TX_TIMEOUT, > > athn_usb_txeof); > > > >athn_usb_txeof() puts the buffer associated with the Tx attempt back onto > >the list of free buffers, and schedules more Tx attempts if needed by > >calling if_start(). > > > >The associated mbuf is freed quite early, before the Tx attempt is even made, > >because the entire packet gets copied into the Tx command sent to the device: > > > > /* Copy payload. */ > > m_copydata(m, 0, m->m_pkthdr.len, frm); > > frm += m->m_pkthdr.len; > > m_freem(m); > > > Hi Stefan! > Reading through athn_usb_txeof, I don't see where it triggers an Rx interrupt > such that the wakeup(9) is done. Additionally, that seems to be a periodic > function whereas I suspect thr interrupt I am looking for is only during > initial of the device, not if-up. But perhaps I am mistaken? I do kot > understand where or how the wakeup(9) occurs. > > Thanks! athn_usb_intr() is not used for Tx interrupts. athn_usb_intr() is only used to process HTC message responses, not for processing packets. The driver sleeps on &ubsc->wait_msg_id when it must wait for a device command to be processed before the driver can continue. A wakeup() is required in that case to interrupt the corresponding tsleep(). But when the driver is submitting a packet for Tx there is no need to wait.
Re: vmd: fix rebooting a received vm
On Sat, May 07, 2022 at 07:58:15AM -0400, Dave Voutila wrote:
> tech@:
>
> Now that vmd only accounts for memory in bytes [1], this fix is a lot
> simpler!
>
> If you use the send/receive functionality and "receive" a sent vm, it
> functions as expected. However, if that vm tries to reboot, it causes
> vmd to exit. (An ipc socket is closed in some error handling and
> triggers a code path ending vmd's event loop.)
>
> The problem was two-fold (and describing it is probably longer than the
> diff itself):
>
> 1. Not un-toggling the VM_RECEIVE_STATE bit on the vm after initial
>launch, triggering "received vm" code paths upon vm reboot.
>
>vmd's "parent" and "vmm" processes *both* track known vm's. The "vmm"
>process removes the vm from its list upon a loss of the child process
>(vm reboot), but the "parent" process keeps it in the tailq and
>reuses it, knowing the vm just requires a restart. (It has to resend
>the vm to the "vmm" process, which sees it as a "new" vm, creating a
>new child process.)
>
> 2. A "received vm" comes with pre-defined memory ranges created when it
>initially booted and these are restored before the vm is resumed. The
>problem is vmd overloads the use of these memory ranges, setting the
>number of ranges to 0 and using the first range's size as a way to
>communicate "max memory" for the vm. Since a clean reboot of a vm
>results in the "parent" process triggering the "vm start" paths, it
>assumes it can use that logic to determine the max memory.
>
>Depending on if you only fix (1) above, the vm results in either
>using the default vm memory (512MB) _or_ the size of the first
>range...which is always 640KB.
>
>Contrary to popular belief, 640KB is not enough for everyone,
>especially our vm.
>
> The diff below resolves (1) in vmd.c's vm_stop() and (2) in config.c's
> config_setvm().
>
> The fact this issue has been present for awhile indicates few people use
> or care about the send/receive functionality. I want to keep the
> functionality in place for awhile longer because I've begun to
> experiment with it *and* it's helping me find other bugs in vmd(8) as
> well as vmm(4). (Expect a vmm diff shortly.)
>
> For anyone looking to test [2], the simplest approach is to create a vm
> without a disk just boot the bsd.rd ramdisk while using a memory value
> that's *not* the default 512m:
>
> # vmctl start -c -b /bsd.rd -m 1g test
>
> Wait for it to give you the installer prompt and then send it to a file:
>
> # vmctl send test > test.vm
>
> You should have a 1g test.vm file. Restore it:
>
> # vmctl receive test < test.vm
>
> Connect to the console and reboot:
>
> # vmctl console test
> (in vm)# reboot
>
> With the diff: the vm reboots and you end up back at the installer
> prompt. `vmctl stat` shows the correct 1g max mem. Reboot at least one
> more time and confirm the same result.
>
> Without the diff: the vmd parent process will exit taking its children
> with it.
>
> ok?
>
reads ok to me, thanks for the explanation. ok mlarkin
> -dv
>
> [1] https://marc.info/?l=openbsd-tech&m=165151507323339&w=2
>
> [2] note that the vmm issue I found means this will work reliably on AMD
> hosts, but may not on Intel hosts. fix coming soon.
>
> diff refs/heads/master refs/heads/vmd-memrange
> blob - 2750be4f580896325e5a3971667c64d61231db06
> blob + cf076cdc27ceaee6e2cbb9cce5825452f0a6
> --- usr.sbin/vmd/config.c
> +++ usr.sbin/vmd/config.c
> @@ -231,6 +231,7 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm, ui
> unsigned int unit;
> struct timeval tv, rate, since_last;
> struct vmop_addr_req var;
> + size_t bytes = 0;
>
> if (vm->vm_state & VM_STATE_RUNNING) {
> log_warnx("%s: vm is already running", __func__);
> @@ -518,6 +519,14 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm, ui
>
> free(tapfds);
>
> + /* Collapse any memranges after the vm was sent to PROC_VMM */
> + if (vcp->vcp_nmemranges > 0) {
> + for (i = 0; i < vcp->vcp_nmemranges; i++)
> + bytes += vcp->vcp_memranges[i].vmr_size;
> + memset(&vcp->vcp_memranges, 0, sizeof(vcp->vcp_memranges));
> + vcp->vcp_nmemranges = 0;
> + vcp->vcp_memranges[0].vmr_size = bytes;
> + }
> vm->vm_state |= VM_STATE_RUNNING;
> return (0);
>
> blob - 4d7e7b5e613723c2166077523dd6e8b9177d6718
> blob + d5d841fd20d9f82e852e3b844ec81d9383713923
> --- usr.sbin/vmd/vmd.c
> +++ usr.sbin/vmd/vmd.c
> @@ -1162,7 +1162,8 @@ vm_stop(struct vmd_vm *vm, int keeptty, const char *ca
> __func__, ps->ps_title[privsep_process], caller,
> vm->vm_vmid, keeptty ? ", keeping tty open" : "");
>
> - vm->vm_state &= ~(VM_STATE_RUNNING | VM_STATE_SHUTDOWN);
> + vm->vm_state &= ~(VM_STATE_RECEIVED | VM_STATE_RUNNING
> + | VM_STATE_SHUTDOWN);
>
> user_inc(&vm->vm_param
vmd: fix rebooting a received vm
tech@:
Now that vmd only accounts for memory in bytes [1], this fix is a lot
simpler!
If you use the send/receive functionality and "receive" a sent vm, it
functions as expected. However, if that vm tries to reboot, it causes
vmd to exit. (An ipc socket is closed in some error handling and
triggers a code path ending vmd's event loop.)
The problem was two-fold (and describing it is probably longer than the
diff itself):
1. Not un-toggling the VM_RECEIVE_STATE bit on the vm after initial
launch, triggering "received vm" code paths upon vm reboot.
vmd's "parent" and "vmm" processes *both* track known vm's. The "vmm"
process removes the vm from its list upon a loss of the child process
(vm reboot), but the "parent" process keeps it in the tailq and
reuses it, knowing the vm just requires a restart. (It has to resend
the vm to the "vmm" process, which sees it as a "new" vm, creating a
new child process.)
2. A "received vm" comes with pre-defined memory ranges created when it
initially booted and these are restored before the vm is resumed. The
problem is vmd overloads the use of these memory ranges, setting the
number of ranges to 0 and using the first range's size as a way to
communicate "max memory" for the vm. Since a clean reboot of a vm
results in the "parent" process triggering the "vm start" paths, it
assumes it can use that logic to determine the max memory.
Depending on if you only fix (1) above, the vm results in either
using the default vm memory (512MB) _or_ the size of the first
range...which is always 640KB.
Contrary to popular belief, 640KB is not enough for everyone,
especially our vm.
The diff below resolves (1) in vmd.c's vm_stop() and (2) in config.c's
config_setvm().
The fact this issue has been present for awhile indicates few people use
or care about the send/receive functionality. I want to keep the
functionality in place for awhile longer because I've begun to
experiment with it *and* it's helping me find other bugs in vmd(8) as
well as vmm(4). (Expect a vmm diff shortly.)
For anyone looking to test [2], the simplest approach is to create a vm
without a disk just boot the bsd.rd ramdisk while using a memory value
that's *not* the default 512m:
# vmctl start -c -b /bsd.rd -m 1g test
Wait for it to give you the installer prompt and then send it to a file:
# vmctl send test > test.vm
You should have a 1g test.vm file. Restore it:
# vmctl receive test < test.vm
Connect to the console and reboot:
# vmctl console test
(in vm)# reboot
With the diff: the vm reboots and you end up back at the installer
prompt. `vmctl stat` shows the correct 1g max mem. Reboot at least one
more time and confirm the same result.
Without the diff: the vmd parent process will exit taking its children
with it.
ok?
-dv
[1] https://marc.info/?l=openbsd-tech&m=165151507323339&w=2
[2] note that the vmm issue I found means this will work reliably on AMD
hosts, but may not on Intel hosts. fix coming soon.
diff refs/heads/master refs/heads/vmd-memrange
blob - 2750be4f580896325e5a3971667c64d61231db06
blob + cf076cdc27ceaee6e2cbb9cce5825452f0a6
--- usr.sbin/vmd/config.c
+++ usr.sbin/vmd/config.c
@@ -231,6 +231,7 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm, ui
unsigned int unit;
struct timeval tv, rate, since_last;
struct vmop_addr_req var;
+ size_t bytes = 0;
if (vm->vm_state & VM_STATE_RUNNING) {
log_warnx("%s: vm is already running", __func__);
@@ -518,6 +519,14 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm, ui
free(tapfds);
+ /* Collapse any memranges after the vm was sent to PROC_VMM */
+ if (vcp->vcp_nmemranges > 0) {
+ for (i = 0; i < vcp->vcp_nmemranges; i++)
+ bytes += vcp->vcp_memranges[i].vmr_size;
+ memset(&vcp->vcp_memranges, 0, sizeof(vcp->vcp_memranges));
+ vcp->vcp_nmemranges = 0;
+ vcp->vcp_memranges[0].vmr_size = bytes;
+ }
vm->vm_state |= VM_STATE_RUNNING;
return (0);
blob - 4d7e7b5e613723c2166077523dd6e8b9177d6718
blob + d5d841fd20d9f82e852e3b844ec81d9383713923
--- usr.sbin/vmd/vmd.c
+++ usr.sbin/vmd/vmd.c
@@ -1162,7 +1162,8 @@ vm_stop(struct vmd_vm *vm, int keeptty, const char *ca
__func__, ps->ps_title[privsep_process], caller,
vm->vm_vmid, keeptty ? ", keeping tty open" : "");
- vm->vm_state &= ~(VM_STATE_RUNNING | VM_STATE_SHUTDOWN);
+ vm->vm_state &= ~(VM_STATE_RECEIVED | VM_STATE_RUNNING
+ | VM_STATE_SHUTDOWN);
user_inc(&vm->vm_params.vmc_params, vm->vm_user, 0);
user_put(vm->vm_user);
Remove fo_poll from struct fileops
Remove unused struct fileops field fo_poll and callbacks.
(After this, VOP_POLL() is next in line for removal.)
OK?
Index: sys/dev/pci/drm/drm_linux.c
===
RCS file: src/sys/dev/pci/drm/drm_linux.c,v
retrieving revision 1.92
diff -u -p -r1.92 drm_linux.c
--- sys/dev/pci/drm/drm_linux.c 1 Mar 2022 11:50:37 - 1.92
+++ sys/dev/pci/drm/drm_linux.c 7 May 2022 10:57:52 -
@@ -2261,12 +2261,6 @@ dmabuf_ioctl(struct file *fp, u_long com
}
int
-dmabuf_poll(struct file *fp, int events, struct proc *p)
-{
- return (0);
-}
-
-int
dmabuf_kqfilter(struct file *fp, struct knote *kn)
{
return (EINVAL);
@@ -2326,7 +2320,6 @@ const struct fileops dmabufops = {
.fo_read= dmabuf_read,
.fo_write = dmabuf_write,
.fo_ioctl = dmabuf_ioctl,
- .fo_poll= dmabuf_poll,
.fo_kqfilter= dmabuf_kqfilter,
.fo_stat= dmabuf_stat,
.fo_close = dmabuf_close,
@@ -2849,12 +2842,6 @@ syncfile_ioctl(struct file *fp, u_long c
}
int
-syncfile_poll(struct file *fp, int events, struct proc *p)
-{
- return 0;
-}
-
-int
syncfile_kqfilter(struct file *fp, struct knote *kn)
{
return EINVAL;
@@ -2908,7 +2895,6 @@ const struct fileops syncfileops = {
.fo_read= syncfile_read,
.fo_write = syncfile_write,
.fo_ioctl = syncfile_ioctl,
- .fo_poll= syncfile_poll,
.fo_kqfilter= syncfile_kqfilter,
.fo_stat= syncfile_stat,
.fo_close = syncfile_close,
Index: sys/kern/kern_event.c
===
RCS file: src/sys/kern/kern_event.c,v
retrieving revision 1.187
diff -u -p -r1.187 kern_event.c
--- sys/kern/kern_event.c 6 May 2022 13:12:16 - 1.187
+++ sys/kern/kern_event.c 7 May 2022 10:57:53 -
@@ -49,7 +50,6 @@
#include
#include
#include
-#include
#include
#include
#include
@@ -79,7 +79,6 @@ int kqueue_read(struct file *, struct ui
intkqueue_write(struct file *, struct uio *, int);
intkqueue_ioctl(struct file *fp, u_long com, caddr_t data,
struct proc *p);
-intkqueue_poll(struct file *fp, int events, struct proc *p);
intkqueue_kqfilter(struct file *fp, struct knote *kn);
intkqueue_stat(struct file *fp, struct stat *st, struct proc *p);
intkqueue_close(struct file *fp, struct proc *p);
@@ -107,7 +106,6 @@ const struct fileops kqueueops = {
.fo_read= kqueue_read,
.fo_write = kqueue_write,
.fo_ioctl = kqueue_ioctl,
- .fo_poll= kqueue_poll,
.fo_kqfilter= kqueue_kqfilter,
.fo_stat= kqueue_stat,
.fo_close = kqueue_close
@@ -1526,25 +1623,6 @@ kqueue_ioctl(struct file *fp, u_long com
}
int
-kqueue_poll(struct file *fp, int events, struct proc *p)
-{
- struct kqueue *kq = (struct kqueue *)fp->f_data;
- int revents = 0;
-
- if (events & (POLLIN | POLLRDNORM)) {
- mtx_enter(&kq->kq_lock);
- if (kq->kq_count) {
- revents |= events & (POLLIN | POLLRDNORM);
- } else {
- selrecord(p, &kq->kq_sel);
- kq->kq_state |= KQ_SEL;
- }
- mtx_leave(&kq->kq_lock);
- }
- return (revents);
-}
-
-int
kqueue_stat(struct file *fp, struct stat *st, struct proc *p)
{
struct kqueue *kq = fp->f_data;
Index: sys/kern/sys_pipe.c
===
RCS file: src/sys/kern/sys_pipe.c,v
retrieving revision 1.137
diff -u -p -r1.137 sys_pipe.c
--- sys/kern/sys_pipe.c 6 May 2022 13:09:41 - 1.137
+++ sys/kern/sys_pipe.c 7 May 2022 10:57:53 -
@@ -40,7 +40,6 @@
#include
#include
#include
-#include
#ifdef KTRACE
#include
#endif
@@ -61,7 +60,6 @@ struct pipe_pair {
intpipe_read(struct file *, struct uio *, int);
intpipe_write(struct file *, struct uio *, int);
intpipe_close(struct file *, struct proc *);
-intpipe_poll(struct file *, int events, struct proc *);
intpipe_kqfilter(struct file *fp, struct knote *kn);
intpipe_ioctl(struct file *, u_long, caddr_t, struct proc *);
intpipe_stat(struct file *fp, struct stat *ub, struct proc *p);
@@ -70,7 +68,6 @@ static const struct fileops pipeops = {
.fo_read= pipe_read,
.fo_write = pipe_write,
.fo_ioctl = pipe_ioctl,
- .fo_poll= pipe_poll,
.fo_kqfilter= pipe_kqfilter,
.fo_stat= pipe_stat,
.fo_close = pipe_close
@@ -719,46 +716,6 @@ pipe_ioctl(struct file *fp, u_long cmd,
}
int
-pipe_poll(struct file *fp, int events, struct proc *p)
-{
- struct pipe *rpipe = fp->f_data, *wpipe;
- struct rwlock *lock = rpipe->pipe_lock;
- int
