Re: tweak msk to avoid ifq_deq_begin/commit/rollback

2017-05-31 Thread Mike Belopuhov
On Wed, May 31, 2017 at 10:28 +1000, David Gwynne wrote:
> ie, do the space check before trying to dequeue and mbuf.
> 
> this also moves it to using m_defrag.
>

Thanks, this looks good.

Forgot to mention that you can remove the
/* now we are committed to transmit the packet */
comment from both sk and msk as it doesn't reveal any sacred
truths anymore.

Same as with sk, I've got no real opinion regarding adding
BUS_DMA_STREAMING, but otherwise I'm OK.

> i dont have an msk plugged in and i dont know how to use the overdrive
> 1000 i have here. if someone could test and ok this, it would be
> great.
>
> Index: if_msk.c
> ===
> RCS file: /cvs/src/sys/dev/pci/if_msk.c,v
> retrieving revision 1.127
> diff -u -p -r1.127 if_msk.c
> --- if_msk.c  10 Apr 2017 02:15:54 -  1.127
> +++ if_msk.c  31 May 2017 00:27:04 -
> @@ -1489,31 +1489,20 @@ msk_encap(struct sk_if_softc *sc_if, str
>  
>   cur = frag = *txidx;
>  
> -#ifdef MSK_DEBUG
> - if (mskdebug >= 2)
> - msk_dump_mbuf(m_head);
> -#endif
> -
> - /*
> -  * Start packing the mbufs in this chain into
> -  * the fragment pointers. Stop when we run out
> -  * of fragments or hit the end of the mbuf chain.
> -  */
> - if (bus_dmamap_load_mbuf(sc->sc_dmatag, txmap, m_head,
> - BUS_DMA_NOWAIT)) {
> - DPRINTFN(2, ("msk_encap: dmamap failed\n"));
> - return (ENOBUFS);
> - }
> -
> - entries = txmap->dm_nsegs * 2;
> - if (entries > (MSK_TX_RING_CNT - sc_if->sk_cdata.sk_tx_cnt - 2)) {
> - DPRINTFN(2, ("msk_encap: too few descriptors free\n"));
> - bus_dmamap_unload(sc->sc_dmatag, txmap);
> - return (ENOBUFS);
> + switch (bus_dmamap_load_mbuf(sc->sc_dmatag, txmap, m_head,
> + BUS_DMA_STREAMING | BUS_DMA_NOWAIT)) {
> + case 0:
> + break;
> + case EFBIG: /* mbuf chain is too fragmented */
> + if (m_defrag(m_head, M_DONTWAIT) == 0 &&
> + bus_dmamap_load_mbuf(sc->sc_dmatag, txmap, m_head,
> + BUS_DMA_STREAMING | BUS_DMA_NOWAIT) == 0)
> + break;
> + /* FALLTHROUGH */
> + default:
> + return (1);
>   }
>  
> - DPRINTFN(2, ("msk_encap: dm_nsegs=%d\n", txmap->dm_nsegs));
> -
>   /* Sync the DMA map. */
>   bus_dmamap_sync(sc->sc_dmatag, txmap, 0, txmap->dm_mapsize,
>   BUS_DMASYNC_PREWRITE);
> @@ -1585,12 +1574,16 @@ msk_start(struct ifnet *ifp)
>   struct sk_if_softc  *sc_if = ifp->if_softc;
>   struct mbuf *m_head = NULL;
>   u_int32_t   idx = sc_if->sk_cdata.sk_tx_prod;
> - int pkts = 0;
> + int post = 0;
>  
> - DPRINTFN(2, ("msk_start\n"));
> + for (;;) {
> + if (sc_if->sk_cdata.sk_tx_cnt + (SK_NTXSEG * 2) + 1 >
> + MSK_TX_RING_CNT) {
> + ifq_set_oactive(>if_snd);
> + break;
> + }
>  
> - while (sc_if->sk_cdata.sk_tx_chain[idx].sk_mbuf == NULL) {
> - m_head = ifq_deq_begin(>if_snd);
> + m_head = ifq_dequeue(>if_snd);
>   if (m_head == NULL)
>   break;
>  
> @@ -1600,14 +1593,11 @@ msk_start(struct ifnet *ifp)
>* for the NIC to drain the ring.
>*/
>   if (msk_encap(sc_if, m_head, )) {
> - ifq_deq_rollback(>if_snd, m_head);
> - ifq_set_oactive(>if_snd);
> - break;
> + m_freem(m_head);
> + continue;
>   }
>  
>   /* now we are committed to transmit the packet */
> - ifq_deq_commit(>if_snd, m_head);
> - pkts++;
>  
>   /*
>* If there's a BPF listener, bounce a copy of this frame
> @@ -1617,18 +1607,17 @@ msk_start(struct ifnet *ifp)
>   if (ifp->if_bpf)
>   bpf_mtap(ifp->if_bpf, m_head, BPF_DIRECTION_OUT);
>  #endif
> + post = 1;
>   }
> - if (pkts == 0)
> + if (post == 0)
>   return;
>  
>   /* Transmit */
> - if (idx != sc_if->sk_cdata.sk_tx_prod) {
> - sc_if->sk_cdata.sk_tx_prod = idx;
> - SK_IF_WRITE_2(sc_if, 1, SK_TXQA1_Y2_PREF_PUTIDX, idx);
> + sc_if->sk_cdata.sk_tx_prod = idx;
> + SK_IF_WRITE_2(sc_if, 1, SK_TXQA1_Y2_PREF_PUTIDX, idx);
>  
> - /* Set a timeout in case the chip goes out to lunch. */
> - ifp->if_timer = MSK_TX_TIMEOUT;
> - }
> + /* Set a timeout in case the chip goes out to lunch. */
> + ifp->if_timer = MSK_TX_TIMEOUT;
>  }
>  
>  void
> 



Re: tweak msk to avoid ifq_deq_begin/commit/rollback

2017-05-30 Thread David Gwynne

> On 31 May 2017, at 10:58, Stuart Henderson  wrote:
> 
> I'll try it tomorrow.. 9 times out of 10 I hit the vp problem and have to 
> reboot though, so it'll be tricky to say whether it fixes what I sometimes 
> see with msk.
> 
> IIRC installing on the overdrive 1000 was just "dd miniroot to a USB stick 
> and boot from it".

it's on jmatthew@s desk. i feel funny about touching it while he's not here.

he did give me access to it though. msk still seems to work. it's surviving cvs 
operations, find / over ssh, and tcpbench, all running concurrently. that's 
usually enough to tickle hairy drivers in my experience.

do you want me to wait for you? or make it easier for you by putting it in now?

dlg


> 
> 
> 
> On 31 May 2017 01:29:06 David Gwynne  wrote:
> 
>> ie, do the space check before trying to dequeue and mbuf.
>> 
>> this also moves it to using m_defrag.
>> 
>> i dont have an msk plugged in and i dont know how to use the overdrive
>> 1000 i have here. if someone could test and ok this, it would be
>> great.
>> 
>> Index: if_msk.c
>> ===
>> RCS file: /cvs/src/sys/dev/pci/if_msk.c,v
>> retrieving revision 1.127
>> diff -u -p -r1.127 if_msk.c
>> --- if_msk.c 10 Apr 2017 02:15:54 -  1.127
>> +++ if_msk.c 31 May 2017 00:27:04 -
>> @@ -1489,31 +1489,20 @@ msk_encap(struct sk_if_softc *sc_if, str
>> 
>>  cur = frag = *txidx;
>> 
>> -#ifdef MSK_DEBUG
>> -if (mskdebug >= 2)
>> -msk_dump_mbuf(m_head);
>> -#endif
>> -
>> -/*
>> - * Start packing the mbufs in this chain into
>> - * the fragment pointers. Stop when we run out
>> - * of fragments or hit the end of the mbuf chain.
>> - */
>> -if (bus_dmamap_load_mbuf(sc->sc_dmatag, txmap, m_head,
>> -BUS_DMA_NOWAIT)) {
>> -DPRINTFN(2, ("msk_encap: dmamap failed\n"));
>> -return (ENOBUFS);
>> -}
>> -
>> -entries = txmap->dm_nsegs * 2;
>> -if (entries > (MSK_TX_RING_CNT - sc_if->sk_cdata.sk_tx_cnt - 2)) {
>> -DPRINTFN(2, ("msk_encap: too few descriptors free\n"));
>> -bus_dmamap_unload(sc->sc_dmatag, txmap);
>> -return (ENOBUFS);
>> +switch (bus_dmamap_load_mbuf(sc->sc_dmatag, txmap, m_head,
>> +BUS_DMA_STREAMING | BUS_DMA_NOWAIT)) {
>> +case 0:
>> +break;
>> +case EFBIG: /* mbuf chain is too fragmented */
>> +if (m_defrag(m_head, M_DONTWAIT) == 0 &&
>> +bus_dmamap_load_mbuf(sc->sc_dmatag, txmap, m_head,
>> +BUS_DMA_STREAMING | BUS_DMA_NOWAIT) == 0)
>> +break;
>> +/* FALLTHROUGH */
>> +default:
>> +return (1);
>>  }
>> 
>> -DPRINTFN(2, ("msk_encap: dm_nsegs=%d\n", txmap->dm_nsegs));
>> -
>>  /* Sync the DMA map. */
>>  bus_dmamap_sync(sc->sc_dmatag, txmap, 0, txmap->dm_mapsize,
>>  BUS_DMASYNC_PREWRITE);
>> @@ -1585,12 +1574,16 @@ msk_start(struct ifnet *ifp)
>>  struct sk_if_softc  *sc_if = ifp->if_softc;
>>  struct mbuf *m_head = NULL;
>>  u_int32_t   idx = sc_if->sk_cdata.sk_tx_prod;
>> -int pkts = 0;
>> +int post = 0;
>> 
>> -DPRINTFN(2, ("msk_start\n"));
>> +for (;;) {
>> +if (sc_if->sk_cdata.sk_tx_cnt + (SK_NTXSEG * 2) + 1 >
>> +MSK_TX_RING_CNT) {
>> +ifq_set_oactive(>if_snd);
>> +break;
>> +}
>> 
>> -while (sc_if->sk_cdata.sk_tx_chain[idx].sk_mbuf == NULL) {
>> -m_head = ifq_deq_begin(>if_snd);
>> +m_head = ifq_dequeue(>if_snd);
>>  if (m_head == NULL)
>>  break;
>> 
>> @@ -1600,14 +1593,11 @@ msk_start(struct ifnet *ifp)
>>   * for the NIC to drain the ring.
>>   */
>>  if (msk_encap(sc_if, m_head, )) {
>> -ifq_deq_rollback(>if_snd, m_head);
>> -ifq_set_oactive(>if_snd);
>> -break;
>> +m_freem(m_head);
>> +continue;
>>  }
>> 
>>  /* now we are committed to transmit the packet */
>> -ifq_deq_commit(>if_snd, m_head);
>> -pkts++;
>> 
>>  /*
>>   * If there's a BPF listener, bounce a copy of this frame
>> @@ -1617,18 +1607,17 @@ msk_start(struct ifnet *ifp)
>>  if (ifp->if_bpf)
>>  bpf_mtap(ifp->if_bpf, m_head, BPF_DIRECTION_OUT);
>> #endif
>> +post = 1;
>>  }
>> -if (pkts == 0)
>> +if (post == 0)
>>  return;
>> 
>>  /* Transmit */
>> -if (idx != sc_if->sk_cdata.sk_tx_prod) {
>> -sc_if->sk_cdata.sk_tx_prod = idx;
>> -SK_IF_WRITE_2(sc_if, 1, SK_TXQA1_Y2_PREF_PUTIDX, idx);
>> +sc_if->sk_cdata.sk_tx_prod = idx;
>> +SK_IF_WRITE_2(sc_if, 

Re: tweak msk to avoid ifq_deq_begin/commit/rollback

2017-05-30 Thread Stuart Henderson
I'll try it tomorrow.. 9 times out of 10 I hit the vp problem and have to 
reboot though, so it'll be tricky to say whether it fixes what I sometimes 
see with msk.


IIRC installing on the overdrive 1000 was just "dd miniroot to a USB stick 
and boot from it".




On 31 May 2017 01:29:06 David Gwynne  wrote:


ie, do the space check before trying to dequeue and mbuf.

this also moves it to using m_defrag.

i dont have an msk plugged in and i dont know how to use the overdrive
1000 i have here. if someone could test and ok this, it would be
great.

Index: if_msk.c
===
RCS file: /cvs/src/sys/dev/pci/if_msk.c,v
retrieving revision 1.127
diff -u -p -r1.127 if_msk.c
--- if_msk.c10 Apr 2017 02:15:54 -  1.127
+++ if_msk.c31 May 2017 00:27:04 -
@@ -1489,31 +1489,20 @@ msk_encap(struct sk_if_softc *sc_if, str

cur = frag = *txidx;

-#ifdef MSK_DEBUG
-   if (mskdebug >= 2)
-   msk_dump_mbuf(m_head);
-#endif
-
-   /*
-* Start packing the mbufs in this chain into
-* the fragment pointers. Stop when we run out
-* of fragments or hit the end of the mbuf chain.
-*/
-   if (bus_dmamap_load_mbuf(sc->sc_dmatag, txmap, m_head,
-   BUS_DMA_NOWAIT)) {
-   DPRINTFN(2, ("msk_encap: dmamap failed\n"));
-   return (ENOBUFS);
-   }
-
-   entries = txmap->dm_nsegs * 2;
-   if (entries > (MSK_TX_RING_CNT - sc_if->sk_cdata.sk_tx_cnt - 2)) {
-   DPRINTFN(2, ("msk_encap: too few descriptors free\n"));
-   bus_dmamap_unload(sc->sc_dmatag, txmap);
-   return (ENOBUFS);
+   switch (bus_dmamap_load_mbuf(sc->sc_dmatag, txmap, m_head,
+   BUS_DMA_STREAMING | BUS_DMA_NOWAIT)) {
+   case 0:
+   break;
+   case EFBIG: /* mbuf chain is too fragmented */
+   if (m_defrag(m_head, M_DONTWAIT) == 0 &&
+   bus_dmamap_load_mbuf(sc->sc_dmatag, txmap, m_head,
+   BUS_DMA_STREAMING | BUS_DMA_NOWAIT) == 0)
+   break;
+   /* FALLTHROUGH */
+   default:
+   return (1);
}

-   DPRINTFN(2, ("msk_encap: dm_nsegs=%d\n", txmap->dm_nsegs));
-
/* Sync the DMA map. */
bus_dmamap_sync(sc->sc_dmatag, txmap, 0, txmap->dm_mapsize,
BUS_DMASYNC_PREWRITE);
@@ -1585,12 +1574,16 @@ msk_start(struct ifnet *ifp)
struct sk_if_softc  *sc_if = ifp->if_softc;
struct mbuf *m_head = NULL;
u_int32_t   idx = sc_if->sk_cdata.sk_tx_prod;
-   int pkts = 0;
+   int post = 0;

-   DPRINTFN(2, ("msk_start\n"));
+   for (;;) {
+   if (sc_if->sk_cdata.sk_tx_cnt + (SK_NTXSEG * 2) + 1 >
+   MSK_TX_RING_CNT) {
+   ifq_set_oactive(>if_snd);
+   break;
+   }

-   while (sc_if->sk_cdata.sk_tx_chain[idx].sk_mbuf == NULL) {
-   m_head = ifq_deq_begin(>if_snd);
+   m_head = ifq_dequeue(>if_snd);
if (m_head == NULL)
break;

@@ -1600,14 +1593,11 @@ msk_start(struct ifnet *ifp)
 * for the NIC to drain the ring.
 */
if (msk_encap(sc_if, m_head, )) {
-   ifq_deq_rollback(>if_snd, m_head);
-   ifq_set_oactive(>if_snd);
-   break;
+   m_freem(m_head);
+   continue;
}

/* now we are committed to transmit the packet */
-   ifq_deq_commit(>if_snd, m_head);
-   pkts++;

/*
 * If there's a BPF listener, bounce a copy of this frame
@@ -1617,18 +1607,17 @@ msk_start(struct ifnet *ifp)
if (ifp->if_bpf)
bpf_mtap(ifp->if_bpf, m_head, BPF_DIRECTION_OUT);
 #endif
+   post = 1;
}
-   if (pkts == 0)
+   if (post == 0)
return;

/* Transmit */
-   if (idx != sc_if->sk_cdata.sk_tx_prod) {
-   sc_if->sk_cdata.sk_tx_prod = idx;
-   SK_IF_WRITE_2(sc_if, 1, SK_TXQA1_Y2_PREF_PUTIDX, idx);
+   sc_if->sk_cdata.sk_tx_prod = idx;
+   SK_IF_WRITE_2(sc_if, 1, SK_TXQA1_Y2_PREF_PUTIDX, idx);

-   /* Set a timeout in case the chip goes out to lunch. */
-   ifp->if_timer = MSK_TX_TIMEOUT;
-   }
+   /* Set a timeout in case the chip goes out to lunch. */
+   ifp->if_timer = MSK_TX_TIMEOUT;
 }

 void






tweak msk to avoid ifq_deq_begin/commit/rollback

2017-05-30 Thread David Gwynne
ie, do the space check before trying to dequeue and mbuf.

this also moves it to using m_defrag.

i dont have an msk plugged in and i dont know how to use the overdrive
1000 i have here. if someone could test and ok this, it would be
great.

Index: if_msk.c
===
RCS file: /cvs/src/sys/dev/pci/if_msk.c,v
retrieving revision 1.127
diff -u -p -r1.127 if_msk.c
--- if_msk.c10 Apr 2017 02:15:54 -  1.127
+++ if_msk.c31 May 2017 00:27:04 -
@@ -1489,31 +1489,20 @@ msk_encap(struct sk_if_softc *sc_if, str
 
cur = frag = *txidx;
 
-#ifdef MSK_DEBUG
-   if (mskdebug >= 2)
-   msk_dump_mbuf(m_head);
-#endif
-
-   /*
-* Start packing the mbufs in this chain into
-* the fragment pointers. Stop when we run out
-* of fragments or hit the end of the mbuf chain.
-*/
-   if (bus_dmamap_load_mbuf(sc->sc_dmatag, txmap, m_head,
-   BUS_DMA_NOWAIT)) {
-   DPRINTFN(2, ("msk_encap: dmamap failed\n"));
-   return (ENOBUFS);
-   }
-
-   entries = txmap->dm_nsegs * 2;
-   if (entries > (MSK_TX_RING_CNT - sc_if->sk_cdata.sk_tx_cnt - 2)) {
-   DPRINTFN(2, ("msk_encap: too few descriptors free\n"));
-   bus_dmamap_unload(sc->sc_dmatag, txmap);
-   return (ENOBUFS);
+   switch (bus_dmamap_load_mbuf(sc->sc_dmatag, txmap, m_head,
+   BUS_DMA_STREAMING | BUS_DMA_NOWAIT)) {
+   case 0:
+   break;
+   case EFBIG: /* mbuf chain is too fragmented */
+   if (m_defrag(m_head, M_DONTWAIT) == 0 &&
+   bus_dmamap_load_mbuf(sc->sc_dmatag, txmap, m_head,
+   BUS_DMA_STREAMING | BUS_DMA_NOWAIT) == 0)
+   break;
+   /* FALLTHROUGH */
+   default:
+   return (1);
}
 
-   DPRINTFN(2, ("msk_encap: dm_nsegs=%d\n", txmap->dm_nsegs));
-
/* Sync the DMA map. */
bus_dmamap_sync(sc->sc_dmatag, txmap, 0, txmap->dm_mapsize,
BUS_DMASYNC_PREWRITE);
@@ -1585,12 +1574,16 @@ msk_start(struct ifnet *ifp)
struct sk_if_softc  *sc_if = ifp->if_softc;
struct mbuf *m_head = NULL;
u_int32_t   idx = sc_if->sk_cdata.sk_tx_prod;
-   int pkts = 0;
+   int post = 0;
 
-   DPRINTFN(2, ("msk_start\n"));
+   for (;;) {
+   if (sc_if->sk_cdata.sk_tx_cnt + (SK_NTXSEG * 2) + 1 >
+   MSK_TX_RING_CNT) {
+   ifq_set_oactive(>if_snd);
+   break;
+   }
 
-   while (sc_if->sk_cdata.sk_tx_chain[idx].sk_mbuf == NULL) {
-   m_head = ifq_deq_begin(>if_snd);
+   m_head = ifq_dequeue(>if_snd);
if (m_head == NULL)
break;
 
@@ -1600,14 +1593,11 @@ msk_start(struct ifnet *ifp)
 * for the NIC to drain the ring.
 */
if (msk_encap(sc_if, m_head, )) {
-   ifq_deq_rollback(>if_snd, m_head);
-   ifq_set_oactive(>if_snd);
-   break;
+   m_freem(m_head);
+   continue;
}
 
/* now we are committed to transmit the packet */
-   ifq_deq_commit(>if_snd, m_head);
-   pkts++;
 
/*
 * If there's a BPF listener, bounce a copy of this frame
@@ -1617,18 +1607,17 @@ msk_start(struct ifnet *ifp)
if (ifp->if_bpf)
bpf_mtap(ifp->if_bpf, m_head, BPF_DIRECTION_OUT);
 #endif
+   post = 1;
}
-   if (pkts == 0)
+   if (post == 0)
return;
 
/* Transmit */
-   if (idx != sc_if->sk_cdata.sk_tx_prod) {
-   sc_if->sk_cdata.sk_tx_prod = idx;
-   SK_IF_WRITE_2(sc_if, 1, SK_TXQA1_Y2_PREF_PUTIDX, idx);
+   sc_if->sk_cdata.sk_tx_prod = idx;
+   SK_IF_WRITE_2(sc_if, 1, SK_TXQA1_Y2_PREF_PUTIDX, idx);
 
-   /* Set a timeout in case the chip goes out to lunch. */
-   ifp->if_timer = MSK_TX_TIMEOUT;
-   }
+   /* Set a timeout in case the chip goes out to lunch. */
+   ifp->if_timer = MSK_TX_TIMEOUT;
 }
 
 void