This patch gets rid of the following warning:
net/9p/trans_rdma.c:594:12: warning: ‘rdma_cancelled’ defined but not used [-Wunused-function]
static int rdma_cancelled(struct p9_client *client, struct p9_req_t *req)
The rdma_cancelled function is not called anywhere in the kernel
Signed-off-by: Andi Shyti <[email protected]>
---
net/9p/trans_rdma.c | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c
index 928f2bb..8f68df5 100644
--- a/net/9p/trans_rdma.c
+++ b/net/9p/trans_rdma.c
@@ -588,17 +588,6 @@ static int rdma_cancel(struct p9_client *client, struct p9_req_t *req)
return 1;
}
-/* A request has been fully flushed without a reply.
- * That means we have posted one buffer in excess.
- */
-static int rdma_cancelled(struct p9_client *client, struct p9_req_t *req)
-{
- struct p9_trans_rdma *rdma = client->trans;
-
- atomic_inc(&rdma->excess_rc);
- return 0;
-}
-
/**
* trans_create_rdma - Transport method for creating atransport instance
* @client: client instance
--
1.8.3.2
From: Andi Shyti <[email protected]>
Date: Mon, 22 Jul 2013 14:59:16 +0200
> This patch gets rid of the following warning:
>
> net/9p/trans_rdma.c:594:12: warning: ?rdma_cancelled? defined but not used [-Wunused-function]
> static int rdma_cancelled(struct p9_client *client, struct p9_req_t *req)
>
> The rdma_cancelled function is not called anywhere in the kernel
>
> Signed-off-by: Andi Shyti <[email protected]>
Applied to net-next, thanks.
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Wed, 2013-07-24 at 15:46 -0700, David Miller wrote:
> From: Andi Shyti <[email protected]>
> > This patch gets rid of the following warning:
> >
> > net/9p/trans_rdma.c:594:12: warning: ‘rdma_cancelled’ defined but not used [-Wunused-function]
> > static int rdma_cancelled(struct p9_client *client, struct p9_req_t *req)
> >
> > The rdma_cancelled function is not called anywhere in the kernel
> >
> > Signed-off-by: Andi Shyti <[email protected]>
>
> Applied to net-next, thanks.
After this patch one might as well revert the rest of commit
80b45261a0b2 ("9P: Add cancelled() to the transport functions.") too. It
seems the entire cancelled callback stuff is now pointless.
As I already asked in https://lkml.org/lkml/2013/7/15/87 : did that
commit "forget to actually hook up rdma_cancelled() into
p9_rdma_trans()?". It does look so to me.
Paul Bolle
From: Paul Bolle <[email protected]>
Date: Thu, 25 Jul 2013 01:09:47 +0200
> On Wed, 2013-07-24 at 15:46 -0700, David Miller wrote:
>> From: Andi Shyti <[email protected]>
>> > This patch gets rid of the following warning:
>> >
>> > net/9p/trans_rdma.c:594:12: warning: ?rdma_cancelled? defined but not used [-Wunused-function]
>> > static int rdma_cancelled(struct p9_client *client, struct p9_req_t *req)
>> >
>> > The rdma_cancelled function is not called anywhere in the kernel
>> >
>> > Signed-off-by: Andi Shyti <[email protected]>
>>
>> Applied to net-next, thanks.
>
> After this patch one might as well revert the rest of commit
> 80b45261a0b2 ("9P: Add cancelled() to the transport functions.") too. It
> seems the entire cancelled callback stuff is now pointless.
>
> As I already asked in https://lkml.org/lkml/2013/7/15/87 : did that
> commit "forget to actually hook up rdma_cancelled() into
> p9_rdma_trans()?". It does look so to me.
If nobody responds to this in the next day or so, feel free to send me
a patch which rips it all out.
No response means they don't care, and neither, therefore, should we.
Thanks.
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
Hi,
David Miller wrote on Wed, Jul 24, 2013 :
> From: Paul Bolle <[email protected]>
> Date: Thu, 25 Jul 2013 01:09:47 +0200
> > After this patch one might as well revert the rest of commit
> > 80b45261a0b2 ("9P: Add cancelled() to the transport functions.") too. It
> > seems the entire cancelled callback stuff is now pointless.
> >
> > As I already asked in https://lkml.org/lkml/2013/7/15/87 : did that
> > commit "forget to actually hook up rdma_cancelled() into
> > p9_rdma_trans()?". It does look so to me.
>
> If nobody responds to this in the next day or so, feel free to send me
> a patch which rips it all out.
>
> No response means they don't care, and neither, therefore, should we.
Well, I do care - but I couldn't find where the trans->cancelled member
function was supposed to be called anyway...
So adding it to the struct and fixing the warning is well and fine, but
if it's still never called in the end I don't see much point and there's
nothing to test.
It's on my loooong todo list of things to look at, but as no other reply
came and the patch was never picked up it kind of dropped in priority.
I'll have another look next week
I guess the worst that could happen would be submitting the
rdma_cancelled function again when this is sorted out :)
Regards,
--
Dominique Martinet
Hard morning, sorry for the double mail.
Dominique Martinet wrote on Thu, Jul 25, 2013 :
> Well, I do care - but I couldn't find where the trans->cancelled member
> function was supposed to be called anyway...
> So adding it to the struct and fixing the warning is well and fine, but
> if it's still never called in the end I don't see much point and there's
> nothing to test.
To be more precise, there's a single call to c->trans_mode->cancelled in
net/9p/client.c, in p9_client_flush, which is called on subfunction returning
-ERESTARTSYS... which never happens as far as I could see.
This will be useful once/if we start working on client recovery, though
- so the function in itself definitely does interest me, and I guess
that thinking about I would have preferred to have the hook added rather
than the function removed.
But there definitely is no hurry to add this cancelled function till
then.
Regards,
--
Dominique Martinet
Hi,
> To be more precise, there's a single call to c->trans_mode->cancelled in
> net/9p/client.c, in p9_client_flush, which is called on subfunction returning
> -ERESTARTSYS... which never happens as far as I could see.
>
> This will be useful once/if we start working on client recovery, though
> - so the function in itself definitely does interest me, and I guess
> that thinking about I would have preferred to have the hook added rather
> than the function removed.
> But there definitely is no hurry to add this cancelled function till
> then.
I would prefer to NACK my patch, it messes up things more than
fixes.
If we really want to get rid of this function we should
completely revert this patch:
80b45261a0b263536b043c5ccfc4ba4fc27c2acc
otherwise, spread in the code, there will be empty references to
the 'cancelled' function (struct p9_trans_module in transport.h
and the one you mentioned).
I would rather prefer Paul's approach to mine, let's just get rid
of the warning :)
Andi
From: Andi Shyti <[email protected]>
Date: Thu, 25 Jul 2013 10:27:41 +0200
> I would prefer to NACK my patch, it messes up things more than
> fixes.
Your patch it already applied to my tree, so you must send me
something which fixes things.
> If we really want to get rid of this function we should
> completely revert this patch:
>
> 80b45261a0b263536b043c5ccfc4ba4fc27c2acc
Such as this.
This patch reverts commit
80b45261a0b263536b043c5ccfc4ba4fc27c2acc
which was implementing a 'cancelled' functionality to notify that
a cancelled request will not be replied.
This implementation was not used anywhere and therefore removed.
Signed-off-by: Andi Shyti <[email protected]>
---
include/net/9p/transport.h | 3 ---
net/9p/client.c | 9 ++-------
2 files changed, 2 insertions(+), 10 deletions(-)
diff --git a/include/net/9p/transport.h b/include/net/9p/transport.h
index d9fa68f..9a36d92 100644
--- a/include/net/9p/transport.h
+++ b/include/net/9p/transport.h
@@ -40,8 +40,6 @@
* @close: member function to discard a connection on this transport
* @request: member function to issue a request to the transport
* @cancel: member function to cancel a request (if it hasn't been sent)
- * @cancelled: member function to notify that a cancelled request will not
- * not receive a reply
*
* This is the basic API for a transport module which is registered by the
* transport module with the 9P core network module and used by the client
@@ -60,7 +58,6 @@ struct p9_trans_module {
void (*close) (struct p9_client *);
int (*request) (struct p9_client *, struct p9_req_t *req);
int (*cancel) (struct p9_client *, struct p9_req_t *req);
- int (*cancelled)(struct p9_client *, struct p9_req_t *req);
int (*zc_request)(struct p9_client *, struct p9_req_t *,
char *, char *, int , int, int, int);
};
diff --git a/net/9p/client.c b/net/9p/client.c
index 8b93cae..ba93bda 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -658,17 +658,12 @@ static int p9_client_flush(struct p9_client *c, struct p9_req_t *oldreq)
/*
* if we haven't received a response for oldreq,
- * remove it from the list, and notify the transport
- * layer that the reply will never arrive.
+ * remove it from the list
*/
- spin_lock(&c->lock);
if (oldreq->status == REQ_STATUS_FLSH) {
+ spin_lock(&c->lock);
list_del(&oldreq->req_list);
spin_unlock(&c->lock);
- if (c->trans_mod->cancelled)
- c->trans_mod->cancelled(c, req);
- } else {
- spin_unlock(&c->lock);
}
p9_free_req(c, req);
--
1.8.3.2
Hi David,
On Thu, Jul 25, 2013 at 10:54:24AM +0200, Andi Shyti wrote:
> This patch reverts commit
>
> 80b45261a0b263536b043c5ccfc4ba4fc27c2acc
>
> which was implementing a 'cancelled' functionality to notify that
> a cancelled request will not be replied.
>
> This implementation was not used anywhere and therefore removed.
did you mean this?
> - spin_lock(&c->lock);
> if (oldreq->status == REQ_STATUS_FLSH) {
> + spin_lock(&c->lock);
> list_del(&oldreq->req_list);
> spin_unlock(&c->lock);
> - if (c->trans_mod->cancelled)
> - c->trans_mod->cancelled(c, req);
I just put the spin_lock inside the if statement, so that it
locks only if the statement it's true.
Andi
From: Andi Shyti <[email protected]>
Date: Thu, 25 Jul 2013 10:54:24 +0200
> This patch reverts commit
>
> 80b45261a0b263536b043c5ccfc4ba4fc27c2acc
>
> which was implementing a 'cancelled' functionality to notify that
> a cancelled request will not be replied.
>
> This implementation was not used anywhere and therefore removed.
>
> Signed-off-by: Andi Shyti <[email protected]>
Applied, thanks Andi.