On Tue, Apr 07, 2020 at 12:36:29PM +0300, Vitaliy Makkoveev wrote:
> pppx_if containing tree and per pppx_dev list are protected by rwlock so
> these splx(9) related dances and commentaries are not actual.
> Also pxd_svcq protected by NET_LOCK().
>
> Index: sys/net/if_pppx.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_pppx.c,v
> retrieving revision 1.81
> diff -u -p -r1.81 if_pppx.c
> --- sys/net/if_pppx.c 7 Apr 2020 07:11:22 -0000 1.81
> +++ sys/net/if_pppx.c 7 Apr 2020 09:06:35 -0000
> @@ -126,7 +126,7 @@ struct pppx_dev {
> struct selinfo pxd_wsel;
> struct mutex pxd_wsel_mtx;
>
> - /* queue of packets for userland to service - protected by splnet */
> + /* queue of packets for userland to service - protected by NET_LOCK() */
> struct mbuf_queue pxd_svcq;
> int pxd_waiting;
> LIST_HEAD(,pppx_if) pxd_pxis;
> @@ -622,7 +622,6 @@ pppx_if_next_unit(void)
>
> rw_assert_wrlock(&pppx_ifs_lk);
>
> - /* this is safe without splnet since we're not modifying it */
> do {
> int found = 0;
> RBT_FOREACH(pxi, pppx_ifs, &pppx_ifs) {
> @@ -842,7 +841,6 @@ pppx_add_session(struct pppx_dev *pxd, s
> pxi->pxi_key.pxik_protocol = req->pr_protocol;
> pxi->pxi_dev = pxd;
>
> - /* this is safe without splnet since we're not modifying it */
> if (RBT_FIND(pppx_ifs, &pppx_ifs, pxi) != NULL) {
> pool_put(pppx_if_pl, pxi);
> error = EADDRINUSE;
> @@ -850,8 +848,7 @@ pppx_add_session(struct pppx_dev *pxd, s
> goto out;
> }
>
> - if (RBT_INSERT(pppx_ifs, &pppx_ifs, pxi) != NULL)
> - panic("%s: pppx_ifs modified while lock was held", __func__);
> + RBT_INSERT(pppx_ifs, &pppx_ifs, pxi);
I would merge this into the RBT_FIND() above like this:
if (RBT_INSERT(pppx_ifs, &pppx_ifs, pxi) != NULL) {
pool_put(pppx_if_pl, pxi);
error = EADDRINUSE;
rw_exit_write(&pppx_ifs_lk);
goto out;
}
This does the same thing but without two lookups.
> LIST_INSERT_HEAD(&pxd->pxd_pxis, pxi, pxi_list);
> rw_exit_write(&pppx_ifs_lk);
>
> @@ -1039,8 +1036,7 @@ pppx_if_destroy(struct pppx_dev *pxd, st
> NET_LOCK();
>
> rw_enter_write(&pppx_ifs_lk);
> - if (RBT_REMOVE(pppx_ifs, &pppx_ifs, pxi) == NULL)
> - panic("%s: pppx_ifs modified while lock was held", __func__);
> + RBT_REMOVE(pppx_ifs, &pppx_ifs, pxi);
> LIST_REMOVE(pxi, pxi_list);
> rw_exit_write(&pppx_ifs_lk);
>
Unsure about this one here. I would prefer if the panic remained for now
(mainly because of the XXXSMP NET_UNLOCK() dance just above). I wonder if the
order of this could not be modified so that the NET_LOCK is released after
the RBT_REMOVE.
--
:wq Claudio
