Date: Mon, 9 Jul 2007 13:02:18 +0900 From: "Hidetoshi Shimokawa" <simokawa@FreeBSD.ORG> To: "Kobayashi Katsushi" <ikob@ni.aist.go.jp> Cc: freebsd-firewire@freebsd.org Subject: Re: Patch for async. packet corruption in polling mode. Message-ID: <626eb4530707082102g3363c14br21555d8f3b1c8385@mail.gmail.com> In-Reply-To: <06F66E24-16E7-4FBD-BF60-C9F5C31064F6@ni.aist.go.jp> References: <9E3627A8-0036-408A-B604-11A514B0CFD2@ni.aist.go.jp> <626eb4530706290218n66064fferfe04a6a146fb69f9@mail.gmail.com> <626eb4530707081930x6ec4ad60q54df347be1aba727@mail.gmail.com> <06F66E24-16E7-4FBD-BF60-C9F5C31064F6@ni.aist.go.jp>
next in thread | previous in thread | raw e-mail | index | archive | help
I need some more experiments to see whether limiting packets
is useful or not. Actually, we need some improvements on TX
scheduling to saturate S400 for my laptop.
If you have any insight abount the effect of interrupt coalescing and
device polling, please let me know.
I'll fix the problem on RELENG_6 before 6.3 anyway.
Thanks for your input.
On 7/9/07, Kobayashi Katsushi <ikob@ni.aist.go.jp> wrote:
> Hi,
>
> Our experience was done on -stable only. I am not sure how
> to work on the latest -current. I understood your suggestion
> that disabling limit number of packet processing works fine
> also in -stable.
>
> If the limit packet processing option has been removed from
> "device polling" in -current, my patch is an useless for at
> least -current. However, if the option continues to be
> supported in -current, the same corruption issue will be
> happen again.
>
> BTW, It is difficult to ask your question why turning on
> polling option. Simply say I would like to evaluate the effect
> of interrupt coalescing and/or device polling.
>
> P.S. I also understood the corruption mainly caused by a
> pretty strange spec. for 1394 OHCI (only support buffer fill
> mode in async. packet receive), and this spec. requires
> more little effort for device deriver implementor.
>
> --
> Katsushi Kobayashi
>
>
>
> On 2007/07/09, at 11:30, Hidetoshi Shimokawa wrote:
>
> > It looks that your patch fixes the case where the number of packets
> > processed
> > is limited. As far as I tested, -current does not have this problem
> > because
> > it doesn't limit the number of packets processed for polling.
> >
> > I assume you are using polling mode for performance, right?
> > I think you should gain performance if fwohci shares IRQ with
> > other devices. Which devices does your machine share IRQ with?
> >
> > We have two choices now,
> > 1. Merge MPSAFE firewire stack from current to RELENG_6.
> > We need some modification because CAM in -stable needs the Giant.
> >
> > 2. Fix RELENG_6 independently.
> > Just removing packet limitation is the easiest way.
> >
> > I'm not sure about effectiveness of limiting the number of packets
> > in preemptive kernel. In my opinion, separating rx and tx taskq seems
> > a good way to go.
> >
> > On 6/29/07, Hidetoshi Shimokawa <simokawa@freebsd.org> wrote:
> >> I remember that there is a PR related this problem.
> >> I'll check your patch next week.
> >>
> >> Thank you for your patch.
> >>
> >> On 6/28/07, Kobayashi Katsushi <ikob@ni.aist.go.jp> wrote:
> >> > Hi,
> >> >
> >> > I met async. packet corruption when using fwip device with
> >> > polling mode. My patch for R6.2 is attached.
> >> >
> >> > Thanks,
> >> >
> >> > --
> >> > Katsushi Kobayashi
> >> >
> >> > Index: sys/dev/firewire/fwohci.c
> >> > ===================================================================
> >> > RCS file: /grid/home/ikob/develop/FreeBSD/sys/dev/firewire/
> >> fwohci.c,v
> >> > retrieving revision 1.1.1.1
> >> > retrieving revision 1.1.1.1.6.3
> >> > diff -c -r1.1.1.1 -r1.1.1.1.6.3
> >> > *** sys/dev/firewire/fwohci.c 22 May 2007 06:25:52 -0000
> >> 1.1.1.1
> >> > --- sys/dev/firewire/fwohci.c 28 Jun 2007 09:09:11 -0000
> >> > 1.1.1.1.6.3
> >> > ***************
> >> > *** 125,131 ****
> >> > static void fwohci_ibr (struct firewire_comm *);
> >> > static void fwohci_db_init (struct fwohci_softc *, struct
> >> > fwohci_dbch *);
> >> > static void fwohci_db_free (struct fwohci_dbch *);
> >> > ! static void fwohci_arcv (struct fwohci_softc *, struct
> >> fwohci_dbch
> >> > *, int);
> >> > static void fwohci_txd (struct fwohci_softc *, struct
> >> fwohci_dbch *);
> >> > static void fwohci_start_atq (struct firewire_comm *);
> >> > static void fwohci_start_ats (struct firewire_comm *);
> >> > --- 125,131 ----
> >> > static void fwohci_ibr (struct firewire_comm *);
> >> > static void fwohci_db_init (struct fwohci_softc *, struct
> >> > fwohci_dbch *);
> >> > static void fwohci_db_free (struct fwohci_dbch *);
> >> > ! static int fwohci_arcv (struct fwohci_softc *, struct fwohci_dbch
> >> > *, int);
> >> > static void fwohci_txd (struct fwohci_softc *, struct
> >> fwohci_dbch *);
> >> > static void fwohci_start_atq (struct firewire_comm *);
> >> > static void fwohci_start_ats (struct firewire_comm *);
> >> > ***************
> >> > *** 580,585 ****
> >> > --- 580,586 ----
> >> > }
> >> >
> >> >
> >> > + sc->pollstat = 0;
> >> > /* Enable interrupts */
> >> > OWRITE(sc, FWOHCI_INTMASK,
> >> > OHCI_INT_ERR | OHCI_INT_PHY_SID
> >> > ***************
> >> > *** 671,676 ****
> >> > --- 672,678 ----
> >> >
> >> > sc->fc.tcode = tinfo;
> >> > sc->fc.dev = dev;
> >> > + sc->pollstat = 0;
> >> >
> >> > sc->fc.config_rom = fwdma_malloc(&sc->fc, CROMSIZE,
> >> CROMSIZE,
> >> > &sc->crom_dma,
> >> > BUS_DMA_WAITOK);
> >> > ***************
> >> > *** 1867,1873 ****
> >> > dump_dma(sc, ARRS_CH);
> >> > dump_db(sc, ARRS_CH);
> >> > #endif
> >> > ! fwohci_arcv(sc, &sc->arrs, count);
> >> > }
> >> > if((stat & OHCI_INT_DMA_PRRQ )){
> >> > #ifndef ACK_ALL
> >> > --- 1869,1879 ----
> >> > dump_dma(sc, ARRS_CH);
> >> > dump_db(sc, ARRS_CH);
> >> > #endif
> >> > ! if(fwohci_arcv(sc, &sc->arrs, count) == 0) {
> >> > ! sc->pollstat &= ~OHCI_INT_DMA_PRRS;
> >> > ! }else{
> >> > ! sc->pollstat |= OHCI_INT_DMA_PRRS;
> >> > ! }
> >> > }
> >> > if((stat & OHCI_INT_DMA_PRRQ )){
> >> > #ifndef ACK_ALL
> >> > ***************
> >> > *** 1877,1883 ****
> >> > dump_dma(sc, ARRQ_CH);
> >> > dump_db(sc, ARRQ_CH);
> >> > #endif
> >> > ! fwohci_arcv(sc, &sc->arrq, count);
> >> > }
> >> > if(stat & OHCI_INT_PHY_SID){
> >> > uint32_t *buf, node_id;
> >> > --- 1883,1893 ----
> >> > dump_dma(sc, ARRQ_CH);
> >> > dump_db(sc, ARRQ_CH);
> >> > #endif
> >> > ! if(fwohci_arcv(sc, &sc->arrq, count) == 0){
> >> > ! sc->pollstat &= ~OHCI_INT_DMA_PRRQ;
> >> > ! }else{
> >> > ! sc->pollstat |= OHCI_INT_DMA_PRRQ;
> >> > ! }
> >> > }
> >> > if(stat & OHCI_INT_PHY_SID){
> >> > uint32_t *buf, node_id;
> >> > ***************
> >> > *** 2081,2086 ****
> >> > --- 2091,2097 ----
> >> > if (1) {
> >> > #endif
> >> > stat = fwochi_check_stat(sc);
> >> > + stat |= sc->pollstat;
> >> > if (stat == 0 || stat == 0xffffffff)
> >> > return;
> >> > }
> >> > ***************
> >> > *** 2601,2607 ****
> >> > return 0;
> >> > }
> >> >
> >> > -
> >> > static int
> >> > fwohci_arcv_swap(struct fw_pkt *fp, int len)
> >> > {
> >> > --- 2612,2617 ----
> >> > ***************
> >> > *** 2686,2692 ****
> >> > dbch->bottom = db_tr;
> >> > }
> >> >
> >> > ! static void
> >> > fwohci_arcv(struct fwohci_softc *sc, struct fwohci_dbch
> >> *dbch, int
> >> > count)
> >> > {
> >> > struct fwohcidb_tr *db_tr;
> >> > --- 2696,2702 ----
> >> > dbch->bottom = db_tr;
> >> > }
> >> >
> >> > ! static int
> >> > fwohci_arcv(struct fwohci_softc *sc, struct fwohci_dbch
> >> *dbch, int
> >> > count)
> >> > {
> >> > struct fwohcidb_tr *db_tr;
> >> > ***************
> >> > *** 2701,2713 ****
> >> > int s;
> >> > caddr_t buf;
> >> > int resCount;
> >> >
> >> > if(&sc->arrq == dbch){
> >> > off = OHCI_ARQOFF;
> >> > }else if(&sc->arrs == dbch){
> >> > off = OHCI_ARSOFF;
> >> > }else{
> >> > ! return;
> >> > }
> >> >
> >> > s = splfw();
> >> > --- 2711,2724 ----
> >> > int s;
> >> > caddr_t buf;
> >> > int resCount;
> >> > + int ret = 0;
> >> >
> >> > if(&sc->arrq == dbch){
> >> > off = OHCI_ARQOFF;
> >> > }else if(&sc->arrs == dbch){
> >> > off = OHCI_ARSOFF;
> >> > }else{
> >> > ! return 0;
> >> > }
> >> >
> >> > s = splfw();
> >> > ***************
> >> > *** 2719,2725 ****
> >> > status = FWOHCI_DMA_READ(db_tr->db[0].db.desc.res) >>
> >> > OHCI_STATUS_SHIFT;
> >> > resCount = FWOHCI_DMA_READ(db_tr->db[0].db.desc.res) &
> >> > OHCI_COUNT_MASK;
> >> > #if 0
> >> > ! printf("status 0x%04x, resCount 0x%04x\n", status,
> >> resCount);
> >> > #endif
> >> > while (status & OHCI_CNTL_DMA_ACTIVE) {
> >> > len = dbch->xferq.psize - resCount;
> >> > --- 2730,2736 ----
> >> > status = FWOHCI_DMA_READ(db_tr->db[0].db.desc.res) >>
> >> > OHCI_STATUS_SHIFT;
> >> > resCount = FWOHCI_DMA_READ(db_tr->db[0].db.desc.res) &
> >> > OHCI_COUNT_MASK;
> >> > #if 0
> >> > ! printf("status 0x%04x, resCount 0x%04x count %d\n", status,
> >> > resCount, count);
> >> > #endif
> >> > while (status & OHCI_CNTL_DMA_ACTIVE) {
> >> > len = dbch->xferq.psize - resCount;
> >> > ***************
> >> > *** 2733,2739 ****
> >> > BUS_DMASYNC_POSTREAD);
> >> > while (len > 0 ) {
> >> > if (count >= 0 && count-- == 0)
> >> > ! goto out;
> >> > if(dbch->pdb_tr != NULL){
> >> > /* we have a fragment in previous
> >> > buffer */
> >> > int rlen;
> >> > --- 2744,2753 ----
> >> > BUS_DMASYNC_POSTREAD);
> >> > while (len > 0 ) {
> >> > if (count >= 0 && count-- == 0)
> >> > ! {
> >> > ! ret = 1;
> >> > ! goto pollout;
> >> > ! }
> >> > if(dbch->pdb_tr != NULL){
> >> > /* we have a fragment in previous
> >> > buffer */
> >> > int rlen;
> >> > ***************
> >> > *** 2898,2906 ****
> >> > --- 2912,2922 ----
> >> > }
> >> > /* XXX make sure DMA is not dead */
> >> > }
> >> > + pollout:
> >> > #if 0
> >> > if (pcnt < 1)
> >> > printf("fwohci_arcv: no packets\n");
> >> > #endif
> >> > splx(s);
> >> > + return ret;
> >> > }
> >> > Index: sys/dev/firewire/fwohcivar.h
> >> > ===================================================================
> >> > RCS file: /grid/home/ikob/develop/FreeBSD/sys/dev/firewire/
> >> fwohcivar.h,v
> >> > retrieving revision 1.1.1.1
> >> > retrieving revision 1.1.1.1.6.1
> >> > diff -c -r1.1.1.1 -r1.1.1.1.6.1
> >> > *** sys/dev/firewire/fwohcivar.h 22 May 2007 06:25:52
> >> > -0000 1.1.1.1
> >> > --- sys/dev/firewire/fwohcivar.h 28 Jun 2007 04:17:22
> >> > -0000 1.1.1.1.6.1
> >> > ***************
> >> > *** 81,86 ****
> >> > --- 81,87 ----
> >> > uint32_t intstat;
> >> > struct task fwohci_task_complete;
> >> > #endif
> >> > + uint32_t pollstat;
> >> > } fwohci_softc_t;
> >> >
> >> > void fwohci_intr (void *arg);
> >> >
> >> >
> >> >
> >> >
> >> >
> >> >
> >> > --
> >> > Katsushi Kobayashi
> >> >
> >> >
> >> >
> >> > _______________________________________________
> >> > freebsd-firewire@freebsd.org mailing list
> >> > http://lists.freebsd.org/mailman/listinfo/freebsd-firewire
> >> > To unsubscribe, send any mail to "freebsd-firewire-
> >> unsubscribe@freebsd.org"
> >> >
> >>
> >>
> >> --
> >> /\ Hidetoshi Shimokawa
> >> \/ simokawa@FreeBSD.ORG
> >>
> >
> >
> > --
> > /\ Hidetoshi Shimokawa
> > \/ simokawa@FreeBSD.ORG
>
> _______________________________________________
> freebsd-firewire@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-firewire
> To unsubscribe, send any mail to "freebsd-firewire-unsubscribe@freebsd.org"
>
--
/\ Hidetoshi Shimokawa
\/ simokawa@FreeBSD.ORG
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?626eb4530707082102g3363c14br21555d8f3b1c8385>