On Thu, Jul 21, 2016 at 12:42:42PM -0500, Steve Wise wrote:
> >
> > On Wed, Jul 20, 2016 at 09:47:50PM -0500, Shiraz Saleem wrote:
> > > On Tue, Jul 19, 2016 at 08:32:53PM +0300, Leon Romanovsky wrote:
> > > > On Tue, Jul 19, 2016 at 09:50:24AM -0500, Shiraz Saleem wrote:
> > > > > On Tue, Jul 19, 2016 at 08:40:06AM +0300, Leon Romanovsky wrote:
> > > > > >
> > > > > > You are the one user of this new inline function.
> > > > > > Why don't you directly call to netlink_unicast() in your
> > > > > > ibnl_unicast()
> > > > > > without messing with widely visible header file?
> > > > >
> > > > > Since there is a non-blocking version of nlmsg_unicast(), the idea is
> > > > > to make a blocking version available to others as well as maintain
> > > > > consistency of existing code.
> > > > >
> > > >
> > > > In such way, please provide patch series which will convert all other
> > > > users to this new call.
> > > >
> > > > ➜ linux-rdma git:(master) grep -rI netlink_unicast * | grep -I 0
> > > > kernel/audit.c: err = netlink_unicast(audit_sock, skb,
> > > > audit_nlk_portid, 0);
> > > > kernel/audit.c: netlink_unicast(aunet->nlsk, skb, dest->portid,
> > > > 0);
> > > > kernel/audit.c: netlink_unicast(aunet->nlsk , reply->skb,
> > > > reply->portid, 0);
> > > > kernel/audit.c: return netlink_unicast(audit_sock, skb,
> > > > audit_nlk_portid, 0);
> > > > samples/connector/cn_test.c: netlink_unicast(nls, skb, 0, 0);
> > >
> > > These usages of netlink_unicast() with blocking are not the same as the
> > > new
> > > nlmsg_unicast_block() function.
> >
> > Really?
> > Did you look in the code?
> > Let's take first function from that grep output
> >
> > 414 err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> > 415 if (err < 0) {
> > ... do something ...
> > 437 } else
> > ... do something else ...
> >
> > which fits nicely with your proposal.
> >
>
> The key is to ensure that places calling a blocking service are never called
> in a non-blocking context. Leon, do you know if the new sites are always
> safe to block?
>
> In general, I think blocking due to sockbuf flow control vs dropping or
> retrying is a good thing for all the users in the rdam core, assuming they
> are safe to block.
Steve,
Sorry for my slow response,
I afraid that you was misled by the author of the proposed patch who did
two logical changes in one patch. One is move from non-blocking mode to
blocking mode which is fine enough after justification was added. And
the second change is introduction of general inline function in common
header file (include/net/netlink.h) with one caller only.
This second change is in question and I'm not feeling comfortable by
half done work.
>
>
> > +static inline int nlmsg_unicast_block(struct sock *sk, struct sk_buff
> > *skb, u32
> > portid)
> > +{
> > + int err;
> > +
> > + err = netlink_unicast(sk, skb, portid, 0);
> > + if (err > 0)
> > + err = 0;
> > +
> > + return err;
> > +}
> >
> >
> > > You can't drop in nlmsg_unicast_block() in
> > > place of netlink_unicast() in these places. I'm not going to introduce
> > > code
> > > which modifies old behavior.
> >
> > Again, you aren't changing any behaviour.
>
> Potential block/sleep is a change. But if we can conclude that these
> additional sites are safe to block, then probably its ok to just go ahead and
> use the blocking service everywhere.
These potential sites has the same blocking call now netlink_unicast(... , ...
, ... , 0),
the difference and question if they can handle normalized return value from new
nlmsg_unicast_block
function. I'm convinced that the answer is yes.
signature.asc
Description: Digital signature
