It doesn't seem to have a problem.  Sorry for my delay.

ok yasuoka

On Wed, 9 Nov 2022 13:24:21 +0300
Vitaliy Makkoveev <m...@openbsd.org> wrote:
> ping...
> 
> On Tue, Nov 01, 2022 at 03:16:02PM +0300, Vitaliy Makkoveev wrote:
>> Push netlock down to pppx_add_session(). The 'pppx_if' structure has
>> the `pxi_ready' member to prevent access to incomplete `pxi', so we
>> don't need to hold netlock during all initialisation process. This
>> removes potential PR_WAITOK/M_WAITOK allocations impact on packet
>> processing. Also this removes relock dances around if_attach() and
>> if_detach() calls.
>> 
>> Do not grab netlock for FIONREAD. mbuf(9) queue doesn't rely on it.
>> 
>> Do not grab netlock around pipex_ioctl() call. pipex(4) has its own
>> protection and doesn't rely on netlock. We need to unlink  pipex(4)
>> session before destroy associated `pxi', it can't be killed
>> concurrently. Also this stops to block packet processing when npppd(8)
>> periodically does PIPEXGCLOSED ioctl(2) commands.
>> 
>> The dummy FIONBIO case doesn't require any lock to be held.
>> 
>> The netlock remains to be taken around pppx_del_session() and
>> pppx_set_session_descr() because pppx(4) data structures rely on it.
>> 
>> Index: sys/net/if_pppx.c
>> ===================================================================
>> RCS file: /cvs/src/sys/net/if_pppx.c,v
>> retrieving revision 1.122
>> diff -u -p -r1.122 if_pppx.c
>> --- sys/net/if_pppx.c        29 Aug 2022 07:51:45 -0000      1.122
>> +++ sys/net/if_pppx.c        1 Nov 2022 10:08:37 -0000
>> @@ -414,7 +414,6 @@ pppxioctl(dev_t dev, u_long cmd, caddr_t
>>      struct pppx_dev *pxd = pppx_dev2pxd(dev);
>>      int error = 0;
>>  
>> -    NET_LOCK();
>>      switch (cmd) {
>>      case PIPEXASESSION:
>>              error = pppx_add_session(pxd,
>> @@ -422,13 +421,17 @@ pppxioctl(dev_t dev, u_long cmd, caddr_t
>>              break;
>>  
>>      case PIPEXDSESSION:
>> +            NET_LOCK();
>>              error = pppx_del_session(pxd,
>>                  (struct pipex_session_close_req *)addr);
>> +            NET_UNLOCK();
>>              break;
>>  
>>      case PIPEXSIFDESCR:
>> +            NET_LOCK();
>>              error = pppx_set_session_descr(pxd,
>>                  (struct pipex_session_descr_req *)addr);
>> +            NET_UNLOCK();
>>              break;
>>  
>>      case FIONBIO:
>> @@ -441,7 +444,6 @@ pppxioctl(dev_t dev, u_long cmd, caddr_t
>>              error = pipex_ioctl(pxd, cmd, addr);
>>              break;
>>      }
>> -    NET_UNLOCK();
>>  
>>      return (error);
>>  }
>> @@ -607,6 +609,7 @@ pppx_add_session(struct pppx_dev *pxd, s
>>  
>>      pxi->pxi_session = session;
>>  
>> +    NET_LOCK();
>>      /* try to set the interface up */
>>      unit = pppx_if_next_unit();
>>      if (unit < 0) {
>> @@ -624,6 +627,7 @@ pppx_add_session(struct pppx_dev *pxd, s
>>              goto out;
>>      }
>>      LIST_INSERT_HEAD(&pxd->pxd_pxis, pxi, pxi_list);
>> +    NET_UNLOCK();
>>  
>>      snprintf(ifp->if_xname, sizeof(ifp->if_xname), "%s%d", "pppx", unit);
>>      ifp->if_mtu = req->pr_peer_mru; /* XXX */
>> @@ -638,13 +642,12 @@ pppx_add_session(struct pppx_dev *pxd, s
>>      /* ifp->if_rdomain = req->pr_rdomain; */
>>      if_counters_alloc(ifp);
>>  
>> -    /* XXXSMP breaks atomicity */
>> -    NET_UNLOCK();
>>      if_attach(ifp);
>> -    NET_LOCK();
>>  
>> +    NET_LOCK();
>>      if_addgroup(ifp, "pppx");
>>      if_alloc_sadl(ifp);
>> +    NET_UNLOCK();
>>  
>>  #if NBPFILTER > 0
>>      bpfattach(&ifp->if_bpf, ifp, DLT_LOOP, sizeof(u_int32_t));
>> @@ -680,6 +683,7 @@ pppx_add_session(struct pppx_dev *pxd, s
>>  
>>      ia->ia_netmask = ia->ia_sockmask.sin_addr.s_addr;
>>  
>> +    NET_LOCK();
>>      error = in_ifinit(ifp, ia, &ifaddr, 1);
>>      if (error) {
>>              printf("pppx: unable to set addresses for %s, error=%d\n",
>> @@ -687,26 +691,29 @@ pppx_add_session(struct pppx_dev *pxd, s
>>      } else {
>>              if_addrhooks_run(ifp);
>>      }
>> +    NET_UNLOCK();
>>  
>>      error = pipex_link_session(session, ifp, pxd);
>>      if (error)
>>              goto detach;
>>  
>> +    NET_LOCK();
>>      SET(ifp->if_flags, IFF_RUNNING);
>>      pxi->pxi_ready = 1;
>> +    NET_UNLOCK();
>>  
>>      return (error);
>>  
>>  detach:
>> -    /* XXXSMP breaks atomicity */
>> -    NET_UNLOCK();
>>      if_detach(ifp);
>> -    NET_LOCK();
>>  
>> +    NET_LOCK();
>>      if (RBT_REMOVE(pppx_ifs, &pppx_ifs, pxi) == NULL)
>>              panic("%s: inconsistent RB tree", __func__);
>>      LIST_REMOVE(pxi, pxi_list);
>>  out:
>> +    NET_UNLOCK();
>> +
>>      pool_put(&pppx_if_pl, pxi);
>>      pipex_rele_session(session);
>>  
> 

Reply via email to