Re: RFC: vioif(4) multiqueue support
Hi all,
I'm going to commit the changes on this weekend.
Regards,
Yamaguchi
2018年12月28日(金) 15:36 Shoichi Yamaguchi :
>
> > > > > https://ftp.netbsd.org/pub/NetBSD/misc/yamaguchi/vioif_mutilq.patch
>
> I have updated the patch.
>
>
> 2018年12月26日(水) 17:20 s ymgch :
> >
> > Thank you for your helpful comments!
> >
> >
> > 2018年12月26日(水) 11:37 Taylor R Campbell
> > :
> > >
> > > > Date: Wed, 26 Dec 2018 10:03:15 +0900
> > > > From: Shoichi Yamaguchi
> > > >
> > > > > I implemented a patch that make vioif(4) support multi-queue. And I
> > > > > have put
> > > > > the patch on ftp.n.o. I used vioif(4) multiqueue on qemu-kvm on Linux
> > > > > kernel
> > > > > 4.19.5. And It seems to be working fine.
> > > > >
> > > >
> > > > Do you have any comments?
> > > > I would like to going to commit the patch if there is no comment until
> > > > tomorrow.
> > >
> > > Hi, Yamaguchi-san! A lot of Americans and Europeans are on vacation
> > > this time of year, so it might be better to hold off for another week
> > > or two. Here's a quick review -- I don't know anything about virtio,
> > > so this is just about use of kernel APIs and abstractions. Someone
> > > who knows something about virtio should take a look too.
> >
> > Yes, indeed. I'll wait for other comments for more one or two week.
> >
> > >
> > > > diff --git a/sys/dev/pci/if_vioif.c b/sys/dev/pci/if_vioif.c
> > > > index 3bbd300e88e..769b108e793 100644
> > > > --- a/sys/dev/pci/if_vioif.c
> > > > +++ b/sys/dev/pci/if_vioif.c
> > > >
> > > > /* Feature bits */
> > > > -#define VIRTIO_NET_F_CSUM(1<<0)
> > > > [...]
> > > > +#define VIRTIO_NET_F_MQ (1<<22)
> > >
> > > If you're going to modify all of the lines here, maybe take the
> > > opportunity to convert them to use __BIT?
> > >
> > > > @@ -171,73 +184,110 @@ struct virtio_net_ctrl_vlan {
> > > > [...]
> > > > /*
> > > > * if_vioifvar.h:
> > > > */
> > > > +struct vioif_txqueue {
> > > > + struct virtqueue*txq_vq;
> > >
> > > Why not make this an embedded structure?
> >
> > The reason why I don't use "struct virtqueue" but use "struct virtqueue *"
> > is to register a virtqueue array ("struct virtqueue []") to virtio(4)
> > as an argument
> > of virtio_child_attach_start() or virtio_child_attach_set_vqs(). Virtqueues
> > used
> > in a child device of virtio(4) like vioif(4) must be array.
> >
> > >
> > > struct vioif_txqueue {
> > > struct virtqueuetxq_vq;
> > > ...
> > > };
> > >
> > > struct vioif_softc {
> > > struct vioif_txqueue*sc_txq;
> > > struct vioif_rxqueue*sc_rxq;
> > > struct vioif_ctrlqueue *sc_ctrlq;
> > > ...
> > > };
> > >
> > > > + kmutex_t*txq_lock;
> > >
> > > Why is this a pointer to kmutex_t with mutex_obj_alloc/free and not
> > > just a kmutex_t with mutex_init/destroy? Is it reused anywhere? If
> > > it is reused, this needs explanation in the comments. If it is not,
> > > just use kmutex_t.
> >
> > It is for the error handling code.
> > Example:
> > for (...) {
> > txq[i]->txq_lock = mutex_obj_alloc();
> > s = softint_establish()
> > if (s == NULL)
> > goto err;
> > }
> >
> > err:
> > for (...)
> > if (txq[i]->txq_lock)
> > mutex_obj_free(txq->txq_lock);
> >
> > I use the pointer value as a flag weather the lock already allocated or
> > not, and
> > I didn't want to add a field into vioif_txqueue to save it.
> >
> > >
> > > Can you write a comment summarizing what locks cover what fields, and,
> > > if more than one lock can be held at once, what the lock order is?
> >
> > I'll add comments for the locks.
> > Currently, the locks cover all fields in the structure, and two or more
> > than locks can't be held at once.
> >
> > >
> > > > +struct vioif_rxqueue {
> > > > + struct virtqueue*rxq_vq;
> > > > + kmutex_t*rxq_lock;
> > >
> > > Likewise.
> > >
> > > > -#define VIOIF_TX_LOCK(_sc) mutex_enter(&(_sc)->sc_tx_lock)
> > > > -#define VIOIF_TX_UNLOCK(_sc) mutex_exit(&(_sc)->sc_tx_lock)
> > > > -#define VIOIF_TX_LOCKED(_sc) mutex_owned(&(_sc)->sc_tx_lock)
> > > > -#define VIOIF_RX_LOCK(_sc) mutex_enter(&(_sc)->sc_rx_lock)
> > > > -#define VIOIF_RX_UNLOCK(_sc) mutex_exit(&(_sc)->sc_rx_lock)
> > > > -#define VIOIF_RX_LOCKED(_sc) mutex_owned(&(_sc)->sc_rx_lock)
> > > > +#define VIOIF_TXQ_LOCK(_q) mutex_enter((_q)->txq_lock)
> > > > +#define VIOIF_TXQ_TRYLOCK(_q)mutex_tryenter((_q)->txq_lock)
> > > > +#define VIOIF_TXQ_UNLOCK(_q) mutex_exit((_q)->txq_lock)
> > > > +#define VIOIF_TXQ_LOCKED(_q) mutex_owned((_q)->txq_lock)
> > > > +
> > > > +#define VIOIF_RXQ_LOCK(_q) mutex_enter((_q)->rxq_lock)
> > > > +#define VIOIF_RXQ_UNLOCK(_q) mutex_exit((_q)->rxq_lock)
> > > > +#define VIOIF_RXQ_LOCKED(_q) mutex_owned((_q)->rxq_lock)
> > >
> > > Can we just use mutex_enter/exit/ without the macros? Sometimes we
> > > use
Re: Unaligned access in kernel on ARMv6+ (Re: CVS commit: src/sys/dev/usb)
On 1/6/2019 5:50 PM, Jason Thorpe wrote: On Jan 6, 2019, at 1:46 PM, matthew green wrote: ... how do we convey the structure alignment needed for softc, or do we fix it by moving it into its own separate aligned allocation? The latter, I think. I agree. When a driver has a strict alignment requirement, it's up to the driver to implement that alignment. The driver can't know for sure that malloc() is going to return memory with any specific alignment considerations, so the driver has to enure the alignment on their own. Patrick Klos
Re: Unaligned access in kernel on ARMv6+ (Re: CVS commit: src/sys/dev/usb)
> On Jan 6, 2019, at 2:26 PM, Christos Zoulas wrote: > > Shouldn't the compiler know how to do this right by padding around the > structure that needs alignment and assuming the default alignment for > the enclosing structure? No, because the compiler can't be assured of what alignment the allocator will return. -- thorpej
Re: Unaligned access in kernel on ARMv6+ (Re: CVS commit: src/sys/dev/usb)
> On Jan 6, 2019, at 1:46 PM, matthew green wrote: > > ... how do we convey the structure alignment needed for > softc, or do we fix it by moving it into its own separate > aligned allocation? The latter, I think. -- thorpej
re: Unaligned access in kernel on ARMv6+ (Re: CVS commit: src/sys/dev/usb)
> | for xhci, all of these seem to be the same issue and it
> | appears to be a "high level allocator doesn't know what
> | it is allocating and does not align properly". the
> | problem is likely:
> |
> | #define XHCI_TRB_ALIGN 16
> | struct xhci_trb {
> | ...
> | } __packed __aligned(XHCI_TRB_ALIGN);
> |
> | which is included in:
> |
> | struct xhci_softc {
> | ...
> | struct xhci_trb sc_result_trb;
> |
> |
> | ... how do we convey the structure alignment needed for
> | softc, or do we fix it by moving it into its own separate
> | aligned allocation?
>
> Shouldn't the compiler know how to do this right by padding around the
> structure that needs alignment and assuming the default alignment for
> the enclosing structure?
how can it? it has to be 16 byte aligned. the alignment
provided is only 8. so sometimes the padding will work and
sometimes the padding will fault, depending on what bit 3
is of the returned address. it may already pad before hand
to get 16 byte alignment from the start of the structure,
as well as force the whole structure alignment to 16, looking
above this member, it's hard to easily tell, it could be
needing 4, 8 or 12 bytes of padding (4/12 only 32 bit only.)
this is why the compiler assumes it must be 16 byte aligned
already -- because that's the only case that is valid.
.mrg.
re: Unaligned access in kernel on ARMv6+ (Re: CVS commit: src/sys/dev/usb)
On Jan 7, 8:46am, m...@eterna.com.au (matthew green) wrote:
-- Subject: re: Unaligned access in kernel on ARMv6+ (Re: CVS commit: src/sys
| > http://netbsd.org/~kamil/kubsan/0007-boot-real-hardware.txt
|
| for xhci, all of these seem to be the same issue and it
| appears to be a "high level allocator doesn't know what
| it is allocating and does not align properly". the
| problem is likely:
|
| #define XHCI_TRB_ALIGN 16
| struct xhci_trb {
| ...
| } __packed __aligned(XHCI_TRB_ALIGN);
|
| which is included in:
|
| struct xhci_softc {
| ...
| struct xhci_trb sc_result_trb;
|
|
| ... how do we convey the structure alignment needed for
| softc, or do we fix it by moving it into its own separate
| aligned allocation?
Shouldn't the compiler know how to do this right by padding around the
structure that needs alignment and assuming the default alignment for
the enclosing structure?
christos
re: Unaligned access in kernel on ARMv6+ (Re: CVS commit: src/sys/dev/usb)
> http://netbsd.org/~kamil/kubsan/0007-boot-real-hardware.txt
for xhci, all of these seem to be the same issue and it
appears to be a "high level allocator doesn't know what
it is allocating and does not align properly". the
problem is likely:
#define XHCI_TRB_ALIGN 16
struct xhci_trb {
...
} __packed __aligned(XHCI_TRB_ALIGN);
which is included in:
struct xhci_softc {
...
struct xhci_trb sc_result_trb;
... how do we convey the structure alignment needed for
softc, or do we fix it by moving it into its own separate
aligned allocation?
.mrg.
Re: Adding new feature - Kcov
On 04.01.2019 19:56, Maxime Villard wrote: > > Interrupt != exception. When a page fault comes in, there's no flag that is > set in proc/lwp/curcpu, so you can't know if you are in an exception > context; > ci_idepth is unrelated. > > Of course we could add such a flag under #ifdef KCOV and then check for > this > flag in __sanitizer_cov_trace_pc. > > But before that, it would be good to make sure that the extra output is > indeed noise (and not something the fuzzer expects). Because a lof of > things > we do in exception context may contain bugs, and we want to fuzz all of > that. > > Maybe check what Linux does? Linux does not print "side effect" routines from virtual memory layer. If there are no longer any concerns, please import it into src/ and remove kcov(4) entry from src/doc/TODO.sanitizers. In future once we will get GCC 8+ we will be able to add additional modes of execution. Clang already supports more, but short term we can delay it. signature.asc Description: OpenPGP digital signature
