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

[zhoucm1 <zhoucm1-5C7GfCeVMHo@public.gmane.org>]

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

how about the attached?

If ok, I will merge to pathc#1.


-David


On 2019年03月21日 22:40, Christian König wrote:
> 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: 25835 bytes --]

[-- Attachment #2: 0001-fix-rcu-warning-from-kernel-robot.patch --]
[-- Type: text/x-patch, Size: 1503 bytes --]

>From 0cb7171b2a99b425323a8e02a968c9488de29608 Mon Sep 17 00:00:00 2001
From: Chunming Zhou <david1.zhou-5C7GfCeVMHo@public.gmane.org>
Date: Fri, 22 Mar 2019 15:33:01 +0800
Subject: [PATCH] fix rcu warning from kernel robot

Signed-off-by: Chunming Zhou <david1.zhou-5C7GfCeVMHo@public.gmane.org>
---
 drivers/dma-buf/dma-fence-chain.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
index 0c5e3c902fa0..c729f98a7bd3 100644
--- a/drivers/dma-buf/dma-fence-chain.c
+++ b/drivers/dma-buf/dma-fence-chain.c
@@ -70,7 +70,7 @@ struct dma_fence *dma_fence_chain_walk(struct dma_fence *fence)
 			replacement = NULL;
 		}
 
-		tmp = cmpxchg(&chain->prev, prev, replacement);
+		tmp = cmpxchg((void **)&chain->prev, (void *)prev, (void *)replacement);
 		if (tmp == prev)
 			dma_fence_put(tmp);
 		else
@@ -187,7 +187,7 @@ static void dma_fence_chain_release(struct dma_fence *fence)
 {
 	struct dma_fence_chain *chain = to_dma_fence_chain(fence);
 
-	dma_fence_put(chain->prev);
+	dma_fence_put(rcu_dereference_protected(chain->prev, true));
 	dma_fence_put(chain->fence);
 	dma_fence_free(fence);
 }
@@ -219,7 +219,7 @@ void dma_fence_chain_init(struct dma_fence_chain *chain,
 	uint64_t context;
 
 	spin_lock_init(&chain->lock);
-	chain->prev = prev;
+	rcu_assign_pointer(chain->prev, prev);
 	chain->fence = fence;
 	chain->prev_seqno = 0;
 	init_irq_work(&chain->work, dma_fence_chain_irq_work);
-- 
2.17.1


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

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