Re: [PATCH 1/9] dma-buf: add new dma_fence_chain container v6

["Christian König" <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 14885 bytes --]

No, atomic cmpxchg is a hardware operation. If you want to replace that 
you need a lock again.

Maybe just add a comment and use an explicit cast to void* ? Not sure if 
that silences the warning.

Christian.

Am 21.03.19 um 15:13 schrieb Zhou, David(ChunMing):
> cmpxchg be replaced by some simple c sentance?
> otherwise we have to remove __rcu of chian->prev.
>
> -David
>
> -------- Original Message --------
> Subject: Re: [PATCH 1/9] dma-buf: add new dma_fence_chain container v6
> From: Christian König
> To: "Zhou, David(ChunMing)" ,kbuild test robot ,"Zhou, David(ChunMing)"
> CC: 
> kbuild-all-JC7UmRfGjtg@public.gmane.org,dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,lionel.g.landwerlin-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org,jason-fQELhIk9awXprZlt/sZkLg@public.gmane.org,"Koenig, 
> Christian" ,"Hector, Tobias"
>
> Hi David,
>
> For the cmpxchg() case I of hand don't know either. Looks like so far
> nobody has used cmpxchg() with rcu protected structures.
>
> The other cases should be replaced by RCU_INIT_POINTER() or
> rcu_dereference_protected(.., true);
>
> Regards,
> Christian.
>
> Am 21.03.19 um 07:34 schrieb zhoucm1:
> > Hi Lionel and Christian,
> >
> > Below is robot report for chain->prev, which was added __rcu as you
> > suggested.
> >
> > How to fix this line "tmp = cmpxchg(&chain->prev, prev, replacement); "?
> > I checked kernel header file, seems it has no cmpxchg for rcu.
> >
> > Any suggestion to fix this robot report?
> >
> > Thanks,
> > -David
> >
> > On 2019年03月21日 08:24, kbuild test robot wrote:
> >> Hi Chunming,
> >>
> >> I love your patch! Perhaps something to improve:
> >>
> >> [auto build test WARNING on linus/master]
> >> [also build test WARNING on v5.1-rc1 next-20190320]
> >> [if your patch is applied to the wrong git tree, please drop us a
> >> note to help improve the system]
> >>
> >> url:
> >> 
> https://github.com/0day-ci/linux/commits/Chunming-Zhou/dma-buf-add-new-dma_fence_chain-container-v6/20190320-223607
> >> reproduce:
> >>          # apt-get install sparse
> >>          make ARCH=x86_64 allmodconfig
> >>          make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
> >>
> >>
> >> sparse warnings: (new ones prefixed by >>)
> >>
> >>>> drivers/dma-buf/dma-fence-chain.c:73:23: sparse: incorrect type in
> >>>> initializer (different address spaces) @@    expected struct
> >>>> dma_fence [noderef] <asn:4>*__old @@    got  dma_fence [noderef]
> >>>> <asn:4>*__old @@
> >>     drivers/dma-buf/dma-fence-chain.c:73:23: expected struct
> >> dma_fence [noderef] <asn:4>*__old
> >>     drivers/dma-buf/dma-fence-chain.c:73:23:    got struct dma_fence
> >> *[assigned] prev
> >>>> drivers/dma-buf/dma-fence-chain.c:73:23: sparse: incorrect type in
> >>>> initializer (different address spaces) @@    expected struct
> >>>> dma_fence [noderef] <asn:4>*__new @@    got  dma_fence [noderef]
> >>>> <asn:4>*__new @@
> >>     drivers/dma-buf/dma-fence-chain.c:73:23: expected struct
> >> dma_fence [noderef] <asn:4>*__new
> >>     drivers/dma-buf/dma-fence-chain.c:73:23:    got struct dma_fence
> >> *[assigned] replacement
> >>>> drivers/dma-buf/dma-fence-chain.c:73:21: sparse: incorrect type in
> >>>> assignment (different address spaces) @@ expected struct
> >>>> dma_fence *tmp @@    got struct dma_fence [noderef] <struct
> >>>> dma_fence *tmp @@
> >>     drivers/dma-buf/dma-fence-chain.c:73:21: expected struct
> >> dma_fence *tmp
> >>     drivers/dma-buf/dma-fence-chain.c:73:21:    got struct dma_fence
> >> [noderef] <asn:4>*[assigned] __ret
> >>>> drivers/dma-buf/dma-fence-chain.c:190:28: sparse: incorrect type in
> >>>> argument 1 (different address spaces) @@ expected struct
> >>>> dma_fence *fence @@    got struct dma_fence struct dma_fence 
> *fence @@
> >>     drivers/dma-buf/dma-fence-chain.c:190:28: expected struct
> >> dma_fence *fence
> >>     drivers/dma-buf/dma-fence-chain.c:190:28: got struct dma_fence
> >> [noderef] <asn:4>*prev
> >>>> drivers/dma-buf/dma-fence-chain.c:222:21: sparse: incorrect type in
> >>>> assignment (different address spaces) @@ expected struct
> >>>> dma_fence [noderef] <asn:4>*prev @@    got [noderef] <asn:4>*prev @@
> >>     drivers/dma-buf/dma-fence-chain.c:222:21: expected struct
> >> dma_fence [noderef] <asn:4>*prev
> >>     drivers/dma-buf/dma-fence-chain.c:222:21: got struct dma_fence
> >> *prev
> >>     drivers/dma-buf/dma-fence-chain.c:235:33: sparse: expression
> >> using sizeof(void)
> >>     drivers/dma-buf/dma-fence-chain.c:235:33: sparse: expression
> >> using sizeof(void)
> >>
> >> vim +73 drivers/dma-buf/dma-fence-chain.c
> >>
> >>      38
> >>      39    /**
> >>      40     * dma_fence_chain_walk - chain walking function
> >>      41     * @fence: current chain node
> >>      42     *
> >>      43     * Walk the chain to the next node. Returns the next fence
> >> or NULL if we are at
> >>      44     * the end of the chain. Garbage collects chain nodes
> >> which are already
> >>      45     * signaled.
> >>      46     */
> >>      47    struct dma_fence *dma_fence_chain_walk(struct dma_fence
> >> *fence)
> >>      48    {
> >>      49        struct dma_fence_chain *chain, *prev_chain;
> >>      50        struct dma_fence *prev, *replacement, *tmp;
> >>      51
> >>      52        chain = to_dma_fence_chain(fence);
> >>      53        if (!chain) {
> >>      54            dma_fence_put(fence);
> >>      55            return NULL;
> >>      56        }
> >>      57
> >>      58        while ((prev = dma_fence_chain_get_prev(chain))) {
> >>      59
> >>      60            prev_chain = to_dma_fence_chain(prev);
> >>      61            if (prev_chain) {
> >>      62                if (!dma_fence_is_signaled(prev_chain->fence))
> >>      63                    break;
> >>      64
> >>      65                replacement =
> >> dma_fence_chain_get_prev(prev_chain);
> >>      66            } else {
> >>      67                if (!dma_fence_is_signaled(prev))
> >>      68                    break;
> >>      69
> >>      70                replacement = NULL;
> >>      71            }
> >>      72
> >>    > 73            tmp = cmpxchg(&chain->prev, prev, replacement);
> >>      74            if (tmp == prev)
> >>      75                dma_fence_put(tmp);
> >>      76            else
> >>      77                dma_fence_put(replacement);
> >>      78            dma_fence_put(prev);
> >>      79        }
> >>      80
> >>      81        dma_fence_put(fence);
> >>      82        return prev;
> >>      83    }
> >>      84    EXPORT_SYMBOL(dma_fence_chain_walk);
> >>      85
> >>      86    /**
> >>      87     * dma_fence_chain_find_seqno - find fence chain node by
> >> seqno
> >>      88     * @pfence: pointer to the chain node where to start
> >>      89     * @seqno: the sequence number to search for
> >>      90     *
> >>      91     * Advance the fence pointer to the chain node which will
> >> signal this sequence
> >>      92     * number. If no sequence number is provided then this is
> >> a no-op.
> >>      93     *
> >>      94     * Returns EINVAL if the fence is not a chain node or the
> >> sequence number has
> >>      95     * not yet advanced far enough.
> >>      96     */
> >>      97    int dma_fence_chain_find_seqno(struct dma_fence **pfence,
> >> uint64_t seqno)
> >>      98    {
> >>      99        struct dma_fence_chain *chain;
> >>     100
> >>     101        if (!seqno)
> >>     102            return 0;
> >>     103
> >>     104        chain = to_dma_fence_chain(*pfence);
> >>     105        if (!chain || chain->base.seqno < seqno)
> >>     106            return -EINVAL;
> >>     107
> >>     108        dma_fence_chain_for_each(*pfence, &chain->base) {
> >>     109            if ((*pfence)->context != chain->base.context ||
> >>     110 to_dma_fence_chain(*pfence)->prev_seqno < seqno)
> >>     111                break;
> >>     112        }
> >>     113        dma_fence_put(&chain->base);
> >>     114
> >>     115        return 0;
> >>     116    }
> >>     117 EXPORT_SYMBOL(dma_fence_chain_find_seqno);
> >>     118
> >>     119    static const char *dma_fence_chain_get_driver_name(struct
> >> dma_fence *fence)
> >>     120    {
> >>     121            return "dma_fence_chain";
> >>     122    }
> >>     123
> >>     124    static const char
> >> *dma_fence_chain_get_timeline_name(struct dma_fence *fence)
> >>     125    {
> >>     126            return "unbound";
> >>     127    }
> >>     128
> >>     129    static void dma_fence_chain_irq_work(struct irq_work *work)
> >>     130    {
> >>     131        struct dma_fence_chain *chain;
> >>     132
> >>     133        chain = container_of(work, typeof(*chain), work);
> >>     134
> >>     135        /* Try to rearm the callback */
> >>     136        if (!dma_fence_chain_enable_signaling(&chain->base))
> >>     137            /* Ok, we are done. No more unsignaled fences 
> left */
> >>     138 dma_fence_signal(&chain->base);
> >>     139        dma_fence_put(&chain->base);
> >>     140    }
> >>     141
> >>     142    static void dma_fence_chain_cb(struct dma_fence *f, struct
> >> dma_fence_cb *cb)
> >>     143    {
> >>     144        struct dma_fence_chain *chain;
> >>     145
> >>     146        chain = container_of(cb, typeof(*chain), cb);
> >>     147        irq_work_queue(&chain->work);
> >>     148        dma_fence_put(f);
> >>     149    }
> >>     150
> >>     151    static bool dma_fence_chain_enable_signaling(struct
> >> dma_fence *fence)
> >>     152    {
> >>     153        struct dma_fence_chain *head = 
> to_dma_fence_chain(fence);
> >>     154
> >>     155        dma_fence_get(&head->base);
> >>     156        dma_fence_chain_for_each(fence, &head->base) {
> >>     157            struct dma_fence_chain *chain =
> >> to_dma_fence_chain(fence);
> >>     158            struct dma_fence *f = chain ? chain->fence : fence;
> >>     159
> >>     160            dma_fence_get(f);
> >>     161            if (!dma_fence_add_callback(f, &head->cb,
> >> dma_fence_chain_cb)) {
> >>     162                dma_fence_put(fence);
> >>     163                return true;
> >>     164            }
> >>     165            dma_fence_put(f);
> >>     166        }
> >>     167        dma_fence_put(&head->base);
> >>     168        return false;
> >>     169    }
> >>     170
> >>     171    static bool dma_fence_chain_signaled(struct dma_fence 
> *fence)
> >>     172    {
> >>     173        dma_fence_chain_for_each(fence, fence) {
> >>     174            struct dma_fence_chain *chain =
> >> to_dma_fence_chain(fence);
> >>     175            struct dma_fence *f = chain ? chain->fence : fence;
> >>     176
> >>     177            if (!dma_fence_is_signaled(f)) {
> >>     178                dma_fence_put(fence);
> >>     179                return false;
> >>     180            }
> >>     181        }
> >>     182
> >>     183        return true;
> >>     184    }
> >>     185
> >>     186    static void dma_fence_chain_release(struct dma_fence *fence)
> >>     187    {
> >>     188        struct dma_fence_chain *chain =
> >> to_dma_fence_chain(fence);
> >>     189
> >>   > 190        dma_fence_put(chain->prev);
> >>     191        dma_fence_put(chain->fence);
> >>     192        dma_fence_free(fence);
> >>     193    }
> >>     194
> >>     195    const struct dma_fence_ops dma_fence_chain_ops = {
> >>     196        .get_driver_name = dma_fence_chain_get_driver_name,
> >>     197        .get_timeline_name = dma_fence_chain_get_timeline_name,
> >>     198        .enable_signaling = dma_fence_chain_enable_signaling,
> >>     199        .signaled = dma_fence_chain_signaled,
> >>     200        .release = dma_fence_chain_release,
> >>     201    };
> >>     202    EXPORT_SYMBOL(dma_fence_chain_ops);
> >>     203
> >>     204    /**
> >>     205     * dma_fence_chain_init - initialize a fence chain
> >>     206     * @chain: the chain node to initialize
> >>     207     * @prev: the previous fence
> >>     208     * @fence: the current fence
> >>     209     *
> >>     210     * Initialize a new chain node and either start a new
> >> chain or add the node to
> >>     211     * the existing chain of the previous fence.
> >>     212     */
> >>     213    void dma_fence_chain_init(struct dma_fence_chain *chain,
> >>     214                  struct dma_fence *prev,
> >>     215                  struct dma_fence *fence,
> >>     216                  uint64_t seqno)
> >>     217    {
> >>     218        struct dma_fence_chain *prev_chain =
> >> to_dma_fence_chain(prev);
> >>     219        uint64_t context;
> >>     220
> >>     221        spin_lock_init(&chain->lock);
> >>   > 222        chain->prev = prev;
> >>
> >> ---
> >> 0-DAY kernel test infrastructure Open Source
> >> Technology Center
> >> https://lists.01.org/pipermail/kbuild-all Intel Corporation
> >
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


[-- Attachment #1.2: Type: text/html, Size: 24369 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx